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 query string to have null #378

Closed
wants to merge 2 commits into from

Conversation

kobaska
Copy link

@kobaska kobaska commented Oct 26, 2016

Fix to allow query string to have null. At the moment doing a find such as below would not work as qs.stringify function would remove the null from the query.

SomeModel.find({where: {name: null}});

Instead using JSON.stringify with encodeURI

@slnode
Copy link

slnode commented Oct 26, 2016

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@slnode
Copy link

slnode commented Oct 26, 2016

Can one of the admins verify this patch?

2 similar comments
@slnode
Copy link

slnode commented Oct 26, 2016

Can one of the admins verify this patch?

@slnode
Copy link

slnode commented Oct 26, 2016

Can one of the admins verify this patch?

@kobaska kobaska changed the title Handling null value in query Fix issue with null value in query Oct 26, 2016
@kobaska kobaska changed the title Fix issue with null value in query Fix query string to have null Oct 26, 2016
@Amir-61
Copy link
Contributor

Amir-61 commented Oct 26, 2016

@slnode test please

@Amir-61
Copy link
Contributor

Amir-61 commented Oct 26, 2016

Just a side note: In LB3.x we have worked on coercion and made both conversion and coercion more strict. null value is accepted only for "object", "array" and "any".
I see you sent this PR against 2.x. Could you please add test to verify the changes.

@Amir-61 Amir-61 self-assigned this Oct 26, 2016
@Amir-61 Amir-61 added the triage label Oct 26, 2016
@horiaradu
Copy link
Contributor

I think this will be fixed with: #325

@gunjpan
Copy link
Contributor

gunjpan commented Oct 28, 2016

@kobaska : Thank you for your pull request. PR# 325, as mentioned by @horiaradu, addresses the same issue and it is ready to land.

Re. your PR, please see the comments here: #325 (comment)

For future reference, please add test cases along with your code to get the PR review started. See the code contribution guidelines for LoopBack here.

Closing in favor of #325.

@gunjpan
Copy link
Contributor

gunjpan commented Oct 28, 2016

cc/ @Amir-61

@kobaska
Copy link
Author

kobaska commented Oct 30, 2016

@gunjpan Yep will do, thanks :)

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.

None yet

6 participants