Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fixed issue where using path parameters but no custom domain breaks#34

Merged
sloev merged 9 commits intosloev:mastersloev/python-lambdarest:masterfrom
svdgraaf:feature/fix-custompath-edgecaseCopy head branch name to clipboard
Jun 14, 2018
Merged

Fixed issue where using path parameters but no custom domain breaks#34
sloev merged 9 commits intosloev:mastersloev/python-lambdarest:masterfrom
svdgraaf:feature/fix-custompath-edgecaseCopy head branch name to clipboard

Conversation

@svdgraaf
Copy link
Contributor

@svdgraaf svdgraaf commented Jun 6, 2018

I just redeployed an api to api gateway without a custom domain, but with path parameters in the url. This broke the parsing. I think this was an edge cased that I missed last time.

This change fixes this issue. It checks if both path and resource are set. If so, it checks if they are the same. If so, it uses resource, if not. It uses path, which will contain all the values for the path parameters.

@svdgraaf svdgraaf changed the title Fixed issue where using path parameters but no custom domain broke Fixed issue where using path parameters but no custom domain breaks Jun 6, 2018
@sloev
Copy link
Owner

sloev commented Jun 7, 2018

I think we need to do some changes to deployment as seen in trustpilot/python-trustpilot#21
Before we can merge this pr.
It seems like something happened with tox recently.
I will look into it

@sloev
Copy link
Owner

sloev commented Jun 7, 2018

support for python 3.3 removed and all tox removed in
#35

@sloev
Copy link
Owner

sloev commented Jun 7, 2018

can you rebase on master? @svdgraaf then it should work

@svdgraaf
Copy link
Contributor Author

svdgraaf commented Jun 7, 2018

@sloev awesome, checks out :)

# resource path, which will contain the actual requested path itself.
# If they are not the same, this is a proxied or custom domain where
# we need to use the event resource
if 'path' in event and event['resource'][0:3] == event['path'][0:3]:
Copy link
Owner

@sloev sloev Jun 7, 2018

Choose a reason for hiding this comment

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

why do you only check the first 3 letters ? @svdgraaf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm, no particular reason. I'll change it, and change it for the first part of the url, that's more accurate.

@sloev
Copy link
Owner

sloev commented Jun 8, 2018

Are you gonna fix the code so the test is happy ? 😊 @svdgraaf

@svdgraaf
Copy link
Contributor Author

@sloev yes, but will be somewhere later this week :)

@svdgraaf
Copy link
Contributor Author

@sloev done ;)

@sloev
Copy link
Owner

sloev commented Jun 13, 2018

thanks, can you, @svdgraaf bump the version as well ? , now only in the version.py file, and remember history

@svdgraaf
Copy link
Contributor Author

on it

@svdgraaf
Copy link
Contributor Author

I bumped the version to 5.0.1

@sloev
Copy link
Owner

sloev commented Jun 14, 2018

sweet

@sloev sloev merged commit 552825e into sloev:master Jun 14, 2018
@svdgraaf svdgraaf deleted the feature/fix-custompath-edgecase branch June 14, 2018 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.