search

strongloop / strong-remoting

Communicate between objects in servers, mobile apps, and other servers.
www.strongloop.com
Other
105 stars 93 forks source link

Stringify query params which are objects as JSON #325

Closed horiaradu closed 7 years ago

horiaradu commented 8 years ago

Stringify query params which are objects in a way in which empty ararys are preserved instead of removed (default querystring implementation).

fix: https://github.com/strongloop/strong-remoting/issues/324

slnode commented 8 years ago

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

slnode commented 8 years ago

Can one of the admins verify this patch?

slnode commented 8 years ago

Can one of the admins verify this patch?

slnode commented 8 years ago

Can one of the admins verify this patch?

horiaradu commented 8 years ago

Any updates on this one?

richardpringle commented 8 years ago

ok to test

richardpringle commented 8 years ago

none of the tests you added are passing on my machine

richardpringle commented 8 years ago

@slnode test please

richardpringle commented 8 years ago

@horiaradu, the change that was made in this PR only affects the behavior of a GET request, where the source of the argument has not been defined, ie the default GET request behavior. Shouldn't we put this change in for any argument sourced from the query string?

Also, what happens if typeof accept.type === 'object' and typeof val === 'object'? You're still coercing the object into a string. Are you saying that any argument coming from the query should be a string? I don't necessarily disagree, I just want to clarify.

horiaradu commented 8 years ago

@richardpringle

What I want to accomplish is to serialize objects as strings when they need to go into the query string.

I only affected the GET request since I encountered this bug when I connected to a loopback API (as described here) and only GET requests were used. I don't have enough knowledge of strong-remoting to know how the other requests are affected, but I can gladly try to help out in order to make this more generic.

richardpringle commented 8 years ago

We should definitely make sure that the change is uniform and that any object parsed from the query string is coerced properly.

If you could test the expected behaviour is the same for multiple verbs (GET and POST for example), that would be ideal.

horiaradu commented 8 years ago

I will replicate the tests for other verbs as well.

On Thu, Aug 25, 2016 at 8:21 PM, Richard Pringle notifications@github.com wrote:

We should definitely make sure that the change is uniform and that any object parsed from the query string is coerced properly.

If you could test the expected behaviour is the same for multiple verbs (GET and POST for example), that would be ideal.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/strongloop/strong-remoting/pull/325#issuecomment-242471066, or mute the thread https://github.com/notifications/unsubscribe-auth/AC_QOApzltVLd6uM2vXbQncBV-8d1zoWks5qjc8zgaJpZM4JRpHd .

thaiat commented 8 years ago

any reason why this is not merged ?

gunjpan commented 8 years ago

@thaiat

any reason why this is not merged ?

Looks like there's pending item on this PR. Also, has merge conflicts.

@horiaradu : Can you please advise if you were able to finish:

I will replicate the tests for other verbs as well.

Please resolve merge conflicts as well.

horiaradu commented 8 years ago

I will resolve the conflicts asap.

On Wed, Oct 19, 2016 at 9:27 PM, Gunjan Pandya notifications@github.com wrote:

@thaiat https://github.com/thaiat

any reason why this is not merged ?

Looks like there's pending item on this PR. Also, has merge conflicts.

@horiaradu https://github.com/horiaradu : Can you please advise if you were able to finish:

I will replicate the tests for other verbs as well.

Please resolve merge conflicts as well.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/strongloop/strong-remoting/pull/325#issuecomment-254899773, or mute the thread https://github.com/notifications/unsubscribe-auth/AC_QOLNjmKytFtxN-b4XZc764YaWwoYHks5q1mESgaJpZM4JRpHd .

horiaradu commented 8 years ago

@gunjpan I've updated the PR.

Check it out and let me know if I have to fix other things.

gunjpan commented 8 years ago

@slnode ok to test

thaiat commented 8 years ago

this should be ported back to latest 2.x version of strong-remoting as well

horiaradu commented 8 years ago

I can take care of that after this PR is merged.

thaiat commented 8 years ago

@horiaradu i needed that in my code asap so i did it there in case that helps you #377

gunjpan commented 8 years ago

Looks like CI is stuck. Restarting tests.

gunjpan commented 8 years ago

@slnode test please

horiaradu commented 7 years ago

I would want to include the functionality with the unit tests as well so I will just cherry pick the commit.

On Mon, Oct 24, 2016 at 4:30 PM, Gunjan Pandya notifications@github.com wrote:

Looks like CI is stuck. Restarting tests.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/strongloop/strong-remoting/pull/325#issuecomment-255740703, or mute the thread https://github.com/notifications/unsubscribe-auth/AC_QOEjOel-j5hl1v240DDbN2ZsyU5Seks5q3LLzgaJpZM4JRpHd .

horiaradu commented 7 years ago

I see that this PR appeared on the same topic: https://github.com/strongloop/strong-remoting/pull/378

Can somebody take care of this PR? It's been here for a while now...

Amir-61 commented 7 years ago

@horiaradu I see @gunjpan is taking care of reviewing this PR. However before that you need to resolve merge conflicts. If this PR gets landed we can close https://github.com/strongloop/strong-remoting/pull/378

Thanks!

horiaradu commented 7 years ago

I don't see any conflicts...

Amir-61 commented 7 years ago

I don't see any conflicts...

Oh it says:

This branch is out-of-date with the base branch

I believe you need to rebase it on top of master.

horiaradu commented 7 years ago

done.

Amir-61 commented 7 years ago

@gunjpan could you PTAL.

gunjpan commented 7 years ago

@horiaradu : I found pull #378 more concise and generalized, because JSON.stringify() would retain numbers as numbers: From node REPL: 

> console.log(JSON.stringify({a: 2}))
{"a":2}

@bajtos : Thoughts?

horiaradu commented 7 years ago

@gunjpan

EDIT:

378 would also work, but it does not add any tests and I think the unit tests are critical for a library such as this.

I've made the changes you suggested.

bajtos commented 7 years ago

@horiaradu : I found pull #378 more concise and generalized, because JSON.stringify() would retain numbers as numbers: From node REPL: 

console.log(JSON.stringify({a: 2}))
{"a":2}

@bajtos : Thoughts?

I think #378 goes in a wrong direction. What happens when the argument value is a string? I think we will end up with ?key="value" in the query, which is definitely not wanted.

> JSON.stringify('text')
'"text"'

This patch looks good to me as it is now, I think the only remaining step is to squash the commits to a single one with a good commit message (guidelines). I'll leave the final call to @gunjpan.

gunjpan commented 7 years ago

@bajtos : I see, thank you for clarifying the case of string

@horiaradu : PTAL at @bajtos' comments above. I'll further review your edits today. Thank you.

horiaradu commented 7 years ago

@gunjpan done

horiaradu commented 7 years ago

@gunjpan check it out.

I will backport this to 2.X and create a PR as soon as this is merged.

gunjpan commented 7 years ago

@horiaradu : Landed. Please backport it against 2.x, and ping me for review. Thank you :)