Skip to content
This repository has been archived by the owner on Jan 11, 2021. It is now read-only.

Support manual setting of protocol and base_path #215

Merged
merged 2 commits into from
Jun 3, 2015

Conversation

joshglick
Copy link
Contributor

While using DRS in our setup, we ran into issue with the protocol and base_path returning the wrong values with our environment.

This was our setup:

Django Application running on EC2 via a Virtual Environment with Gunicorn, NGINX forced to connect over HTTP.

I've added two optional settings for 'protocol' and 'base_path' that can be provided to manually set these fields for people running similar setups to ours.

I believe this might address issues 83 and 77.

@joshglick joshglick closed this Feb 10, 2015
@joshglick
Copy link
Contributor Author

Failing travis unit test, will reopen after passing

@joshglick joshglick reopened this Feb 10, 2015
@joshglick joshglick closed this Feb 10, 2015
@joshglick
Copy link
Contributor Author

Now passing lint tests. Fixed a bug with the default use case

@joshglick joshglick reopened this Feb 10, 2015
@ariovistus
Copy link
Contributor

could you squash your commits? it makes it easier to review

…g optional settings`

Squashed commit of the following:

commit 401f187
Author: Josh Glick <[email protected]>
Date:   Tue Feb 10 16:55:52 2015 -0500

    Fixed lint formatting issues

commit 9c70795
Author: Josh Glick <[email protected]>
Date:   Tue Feb 10 14:28:17 2015 -0500

    Added parens around or if else statement

commit e11ec08
Author: Josh Glick <[email protected]>
Date:   Tue Feb 10 12:28:29 2015 -0500

    Fixed a bug with default behaviour when default pass wasn't given

commit 3222f25
Author: Josh Glick <[email protected]>
Date:   Mon Feb 9 16:20:57 2015 -0500

    Update full_base_path method to respect protocol property even when base_path not given

commit 06875a7
Author: Josh Glick <[email protected]>
Date:   Mon Feb 9 16:05:11 2015 -0500

    Cleaned up scheme / base_path settings to make them cleaner and more distinct

commit 575a9ac
Author: Josh Glick <[email protected]>
Date:   Mon Feb 9 15:51:59 2015 -0500

    Revert 285a779

commit 285a779
Author: Josh Glick <[email protected]>
Date:   Mon Feb 9 15:32:01 2015 -0500

    Attempt to remove scheme setting

commit e754b2e
Author: Josh Glick <[email protected]>
Date:   Mon Feb 9 15:27:52 2015 -0500

    Updated Resource View to support no base_path provided

commit 8bd8e95
Author: Josh Glick <[email protected]>
Date:   Mon Feb 9 11:59:03 2015 -0500

    Better make use of base_path

commit ee6e4a6
Author: Josh Glick <[email protected]>
Date:   Mon Feb 9 11:54:30 2015 -0500

    Updated the discovery url to be based of base_path if provided

commit eb37416
Author: Josh Glick <[email protected]>
Date:   Mon Feb 9 11:45:02 2015 -0500

    Attempting to reproduce suggestion from marcgibbons#77

commit 604959a
Author: Josh Glick <[email protected]>
Date:   Mon Feb 9 11:21:36 2015 -0500

    Changed how we check for swagger scheme

commit b2ed600
Author: Josh Glick <[email protected]>
Date:   Mon Feb 9 11:13:11 2015 -0500

    Support manual setting of scheme
@joshglick
Copy link
Contributor Author

Done!

@ariovistus
Copy link
Contributor

could you add protocol and base_path to the docs?
could you add a test or two to the suite?

@joshglick
Copy link
Contributor Author

I have updated the documentation and will take a look at the best way to add tests for this this week. Thank you!

@laurentes
Copy link

Any news regarding this pull request? I'm trying to get django-rest-swagger working for this project. However, the default https entry point keeps using http instead of https. I guess that being able to force the protocol to https would fix my issue. Thanks a lot.

ariovistus pushed a commit that referenced this pull request Jun 3, 2015
Support manual setting of protocol and base_path
@ariovistus ariovistus merged commit f893cda into marcgibbons:master Jun 3, 2015
ariovistus pushed a commit that referenced this pull request Jun 3, 2015
@ariovistus
Copy link
Contributor

Well, that was a bit of a bugger to figure out how to test.

@@ -109,6 +116,13 @@ def get(self, request):
}),
})

def get_base_path(self):
protocol = SWAGGER_SETTINGS.get('protocol', 'http')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch is breaking backward compatibility because request.is_secure() is ignored and http is always used unless I add {'protocol': 'https'} in my settings.

request.is_secure() should be the only reliable and consistent way to go here.

I do not understand why adding protocol in swagger settings is a good idea?
You want to use http even if request.is_secure() is True ?
If nginx is forced to connect with http and if request.is_secure() return True, it means you probably have something misconfigured in your settings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also inconsistent with the provided docs. The one case where I was foggily under the impression this would be beneficial is when is_secure is false but you do want https.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants