Stringify query params which are objects as JSON #325

horiaradu · 2016-07-21T10:10:19Z

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

fix: #324

slnode · 2016-07-21T10:10:20Z

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

slnode · 2016-07-21T10:10:20Z

Can one of the admins verify this patch?

slnode · 2016-07-21T10:10:20Z

Can one of the admins verify this patch?

slnode · 2016-07-21T10:10:20Z

Can one of the admins verify this patch?

horiaradu · 2016-08-08T14:08:20Z

Any updates on this one?

richardpringle · 2016-08-11T17:40:02Z

ok to test

richardpringle · 2016-08-11T17:47:38Z

none of the tests you added are passing on my machine

richardpringle · 2016-08-15T18:56:46Z

@slnode test please

richardpringle · 2016-08-15T19:16:31Z

@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 · 2016-08-25T06:36:49Z

@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 · 2016-08-25T17:21:54Z

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 · 2016-08-25T17:22:56Z

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
#325 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AC_QOApzltVLd6uM2vXbQncBV-8d1zoWks5qjc8zgaJpZM4JRpHd
.

thaiat · 2016-10-19T14:02:55Z

any reason why this is not merged ?

gunjpan · 2016-10-19T18:27:28Z

@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 · 2016-10-19T19:32:24Z

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
#325 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AC_QOLNjmKytFtxN-b4XZc764YaWwoYHks5q1mESgaJpZM4JRpHd
.

horiaradu · 2016-10-20T18:00:34Z

@gunjpan I've updated the PR.

merge conflicts resolved
added a couple of specs for POST methods

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

gunjpan · 2016-10-20T19:47:36Z

@slnode ok to test

thaiat · 2016-10-24T03:15:18Z

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

horiaradu · 2016-10-24T07:15:08Z

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

thaiat · 2016-10-24T09:03:57Z

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

gunjpan · 2016-10-24T13:30:26Z

Looks like CI is stuck. Restarting tests.

gunjpan · 2016-10-24T13:30:33Z

@slnode test please

horiaradu · 2016-10-25T18:51:33Z

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
#325 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AC_QOEjOel-j5hl1v240DDbN2ZsyU5Seks5q3LLzgaJpZM4JRpHd
.

horiaradu · 2016-10-26T15:34:39Z

I see that this PR appeared on the same topic: #378

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

Amir-61 · 2016-10-26T15:41:42Z

@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 #378

Thanks!

horiaradu · 2016-10-26T19:34:13Z

I don't see any conflicts...

Amir-61 · 2016-10-26T19:37:02Z

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 · 2016-10-26T19:39:41Z

done.

Amir-61 · 2016-10-26T19:43:48Z

@gunjpan could you PTAL.

gunjpan · 2016-10-26T20:51:31Z

lib/http-invocation.js

+        query[name] = JSON.stringify(val);
+      } else {
+        query[name] = val;
+      }


@horiaradu : This needs to be applied for query case in here as well:
https://github.com/strongloop/strong-remoting/blob/master/lib/http-invocation.js#L127-L132

Better solution would be to extract it in a common function.

gunjpan · 2016-10-26T20:53:18Z

test/http-invocation.test.js

+        return [createEndpoint()];
+      };
+      var inv = givenInvocation(method, { ctorArgs: [], args: [], baseUrl: 'http://base' });
+


line 199:203 could be extracted to a single function as I see them being repeated.

gunjpan · 2016-10-26T20:55:29Z

@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 · 2016-10-27T03:21:03Z

@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 · 2016-10-27T11:25:32Z

@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 · 2016-10-27T12:46:05Z

@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 · 2016-10-28T08:51:32Z

@gunjpan done

gunjpan

@horiaradu : Great workaround! The patch LGTM. Please address minor nitpicks and it will be ready to land. No further review necessary, and I'll land it once updated (please rebase it to a single commit against master after you push the changes).

Once this is landed, it should be backported to 2.x

gunjpan · 2016-10-28T13:44:00Z

test/http-invocation.test.js

@@ -193,6 +193,146 @@ describe('HttpInvocation', function() {
      inv.transformResponse(res, body, cb);
    }
  });
+
+  describe('createRequest', function() {
+    it('should create a simple request', function() {


@horiaradu : Please change all the it cases to reflect imperative mood as described in the Code style guidelines here.

gunjpan · 2016-10-28T13:47:03Z

test/http-invocation.test.js

+  describe('createRequest', function() {
+    it('should create a simple request', function() {
+      var inv = givenInvocationForEndpoint(null, []);
+


Please remove extra line here and
group together var declarations, for example:

var a = 'Hello'; var b = 'How are you'; function printMessage(a, b){ console.log(a + ', ' + b); }

Same applies to other it blocks

Stringify query params which are objects in a way in which empty ararys and null values are preserved instead of removed (default querystring implementation). fix: #324

horiaradu · 2016-10-29T07:52:30Z

@gunjpan check it out.

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

gunjpan · 2016-10-31T12:59:59Z

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

relicense as Artistic-2.0 only update/insert copyright headers Set to no compression when using change stream Implement getEndpoints() * Implement getEndpoints * Deprecate getHttpMethod * Deprecate getFullPath 2.27.0 * Implement getEndpoints() (Amir Jafarian) * Set to no compression when using change stream (Candy) * update/insert copyright headers (Ryan Graham) * relicense as Artistic-2.0 only (Ryan Graham) * Handle array of errors. (Richard Pringle) Add integration tests for coercion in REST Implement comprehensive test suite describing coercion of input arguments in the REST adapter. The test-suite produces test/rest-coercion/report.csv that can be used to compare results across different strong-remoting versions. [back-port of strongloop#304] Fix integration tests for coercion in REST Address style violations, fix bug discovered by jshint. Add http source "formData" Add "formData" as an alias for "form", to make remoting metadata close to Swagger. [back-port of 2e4a500 and aa0bda6] Prioritise auth errors over sharedCtor errors Return 401 Not Authorized when making an unauthorised request to a prototype method for a model instance that does not exist. Potentially breaking change affecting prototype methods: Hooks registered before "invoke" phase (i.e. "auth" phase and any custom phases inserted before "invoke") will be called even when the model instance was not found. The property "ctx.instance" will be undefined in such case, hook handlers must account for this case. 2.28.0 * Prioritise auth errors over sharedCtor errors (Miroslav Bajtoš) * Add http source "formData" (Fabian Off) * Fix integration tests for coercion in REST (Miroslav Bajtoš) * Add integration tests for coercion in REST (Miroslav Bajtoš) 2.28.1 Introduce support for integer datatype There is single datatype in Javascript for representing numbers: i.e. `numbers`. This is a patch to fix strong-remoting#317 - a feature to support data type: `integer`. Includes following changes: - Update `convertValueToTargetType()` & `convertToBasicRemotingType()` - Update `SharedMethod.getType()` - Make `integer` check strict by-default. i.e. If argument and/or returned values are decimal or `> MAX_SAFE_INTEGER`, for argument: send `400`, for returns: send `500` - Rework `convert` method to ignore all arrays except "array of integers" - Add tests to rest-coercion integration tests - Update test cases for integer data type - Update shared-method.js to include support for Node v.0.10.x 2.29.0 * Introduce support for integer datatype (gunjpan) Backport of strongloop#329 Revert globalization of assert() messages Remove the usage of deprecated methods Backport from strong-remoting/strongloop#333 Add new API for finding/disabling remote methods 2.30.0 * Add new API for finding/disabling remote methods (Candy) * Remove the usage of deprecated methods (Amir Jafarian) * Revert globalization of assert() messages (Miroslav Bajtoš) * Backport of strongloop#329 (deepakrkris) Update dependencies 2.31.0 * Update dependencies (Miroslav Bajtoš) Fix tests to use res.get instead of res.headers The latter is case-sensitive and expects header names in lower-case. Backport strongloop#345 Add rest-coercion tests for zero-prefixed numbers Don't convert arg values returned by http function When an argument provides "http" mapping as a custom function, then we should not traverse the return value to detect buffers and dates. The return value could be a complex object, e.g. the full remoting context, in which case the traversal takes too long (in order of second!). Deprecate built-in CORS middleware Push the responsibility of enabling/configuring CORS back to the application developer. Add translation files Update translation files - round#2 Fix support for hooks returning a Promise Fix the code to correctly recurse and invoke next hooks after the promise returned by the first hook callback was resolved. 2.32.0 * Fix support for hooks returning a Promise (Miroslav Bajtoš) * Update translation files - round#2 (Candy) * Add translation files (Amir Jafarian) * Deprecate built-in CORS middleware (Miroslav Bajtoš) * Don't convert arg values returned by http function (Miroslav Bajtoš) * Add rest-coercion tests for zero-prefixed numbers (Miroslav Bajtoš) * Fix tests to use res.get instead of res.headers (Miroslav Bajtoš) Update ja translation file 2.32.1 * Update ja translation file (Candy) Distinguish between "file" and "File" result types Fix regression introduced by a85068e where we started to treat "File" models as the new built-in type "file". In this commit, the implementation is fixed to recognize only the following form as the built-in file: { type: 'file' } All other spellings, most notably "File", are treated as custom types (models) now. 2.32.2 * Distinguish between "file" and "File" result types (Fabien Franzen) lockdown dependency for http-auth Due to a recent release of 2.5.10 which broke node 10/12 support this commit locks down dependency before 2.5.11 is released as some registry will still pick up unpublished version (2.5.10) 2.4.11 is released with apache-md5, apache-crypt locked down See http-auth/http-auth/commit/0097ed6195986942a65e14b38b3f30281c9f6435 Stop changing read-only Number property Rework the code defining missing ES5/ES6 Number properties to not touch the existing properties. This is needed to prevent errors like the following: TypeError: Cannot assign to read only property 'MAX_SAFE_INTEGER' of function Number() { [native code] }"] 2.32.3 * lockdown dependency for http-auth (David Cheung) * Stop changing read-only Number property (Kogulan Baskaran) Stringify object query params as JSON [2.x] Stringify query params which are objects in a way in which empty ararys and null values are preserved instead of removed (default querystring implementation). backport of: strongloop#325 Rest-adapter errorHandler set content-type `json` the defaultHandler always res.sends an object therefore make sense to always set response header as such Release LB2 LTS Enable remote methods to be disabled by alias 2.33.0 * Enable remote methods to be disabled by alias (Rémi Bèges) * Release LB2 LTS (Simon Ho) * Rest-adapter errorHandler set content-type `json` (David Cheung) * Stringify object query params as JSON [2.x] (Horia Radu) Cache getRestMethodByName This function in RestAdapter is slow due to creating RestMethods each time. This can be cached initially, and rechecked if the entry was not found. add unit test updating for review

crandmck added the #community contribution label Jul 21, 2016

horiaradu mentioned this pull request Oct 26, 2016

Fix query string to have null #378

Closed

gunjpan reviewed Oct 26, 2016

View reviewed changes

gunjpan self-assigned this Oct 28, 2016

gunjpan added the bug label Oct 28, 2016

gunjpan approved these changes Oct 28, 2016

View reviewed changes

gunjpan mentioned this pull request Oct 28, 2016

Fix issue with serialization #377

Closed

Stringify query params which are objects as JSON

d434e4e

Stringify query params which are objects in a way in which empty ararys and null values are preserved instead of removed (default querystring implementation). fix: #324

gunjpan merged commit bd047c8 into strongloop:master Oct 31, 2016

gunjpan removed the #community contribution label Oct 31, 2016

horiaradu mentioned this pull request Nov 1, 2016

Stringify query params which are objects as JSON [2.x] #381

Merged

This was referenced Jun 6, 2021

Update dependency loopback to v2.42.0 [SECURITY] duyet/reviews-mining#26

Open

fix(deps): update dependency loopback to v2.42.0 [security] duyet/thesis#26

Merged

Stringify query params which are objects as JSON #325

Stringify query params which are objects as JSON #325

Conversation

horiaradu commented Jul 21, 2016

slnode commented Jul 21, 2016

slnode commented Jul 21, 2016

slnode commented Jul 21, 2016

slnode commented Jul 21, 2016

horiaradu commented Aug 8, 2016

richardpringle commented Aug 11, 2016

richardpringle commented Aug 11, 2016

richardpringle commented Aug 15, 2016

richardpringle commented Aug 15, 2016 • edited

horiaradu commented Aug 25, 2016

richardpringle commented Aug 25, 2016

horiaradu commented Aug 25, 2016

thaiat commented Oct 19, 2016

gunjpan commented Oct 19, 2016

horiaradu commented Oct 19, 2016

horiaradu commented Oct 20, 2016

gunjpan commented Oct 20, 2016

thaiat commented Oct 24, 2016

horiaradu commented Oct 24, 2016

thaiat commented Oct 24, 2016

gunjpan commented Oct 24, 2016

gunjpan commented Oct 24, 2016

horiaradu commented Oct 25, 2016

horiaradu commented Oct 26, 2016

Amir-61 commented Oct 26, 2016

horiaradu commented Oct 26, 2016

Amir-61 commented Oct 26, 2016

horiaradu commented Oct 26, 2016

Amir-61 commented Oct 26, 2016

gunjpan Oct 26, 2016

Choose a reason for hiding this comment

gunjpan Oct 26, 2016

Choose a reason for hiding this comment

gunjpan commented Oct 26, 2016 • edited

horiaradu commented Oct 27, 2016 • edited

bajtos commented Oct 27, 2016

gunjpan commented Oct 27, 2016

horiaradu commented Oct 28, 2016 • edited

gunjpan left a comment

Choose a reason for hiding this comment

gunjpan Oct 28, 2016

Choose a reason for hiding this comment

gunjpan Oct 28, 2016

Choose a reason for hiding this comment

horiaradu commented Oct 29, 2016

gunjpan commented Oct 31, 2016

richardpringle commented Aug 15, 2016 •

edited

gunjpan commented Oct 26, 2016 •

edited

horiaradu commented Oct 27, 2016 •

edited

horiaradu commented Oct 28, 2016 •

edited