Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #505 dynamodb value must not be empty #509

Conversation

eduardo11010112
Copy link

Changed filtering of client data by using empty() instead of is_null() in the setAccessToken, setAuthorizationCode and setRefreshToken methods of the DynamoDB storage..
Changed the getDefaultScope method to return null if there is no default scope set in the database.
Added tests for these changes.

@bshaffer
Copy link
Owner

bshaffer commented Mar 3, 2015

two thoughts on this PR... and sorry it has taken me so long to review it...

  1. If we DO want to pass a null value to dynamo DB (i.e. set the client_secret or scope value to null) will this implementation allow us to do this?
  2. can you squash these changes into a single commit using git rebase -i HEAD~3 ?

Thank you!

…b storage.

Adding test for dynamodb storage.
Default scope should be null when none is set in the database.

Removing immediate return true in dynamodb test.
@eduardo11010112 eduardo11010112 force-pushed the fix-#505-dynamodb-value-must-not-be-empty branch from bca40bc to 0786a36 Compare March 4, 2015 09:25
@eduardo11010112
Copy link
Author

not a problem. thanks for looking into it.

  1. we are using it now on a project we are working on. I'd say yes, this implementation allows us to let the client_secret and/or scope have null values. It fixes the error in a situation where the scope is null and there's no default scope defined in the database.
  2. i'm glad to squash the commits into one. (my first time to do it)

bshaffer added a commit that referenced this pull request Mar 24, 2015
…be-empty

Fix #505 dynamodb value must not be empty
@bshaffer bshaffer merged commit e7c42e1 into bshaffer:develop Mar 24, 2015
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