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

Correct Query String Serialization and Deserialization #14174

Closed
nathanhammond opened this issue Aug 31, 2016 · 15 comments · Fixed by #19236
Closed

Correct Query String Serialization and Deserialization #14174

nathanhammond opened this issue Aug 31, 2016 · 15 comments · Fixed by #19236
Labels

Comments

@nathanhammond
Copy link
Member

nathanhammond commented Aug 31, 2016

This bug supersedes #13816 and #14171 (and who knows how many others). The behavior for serializing objects to query strings inside of Ember is wrong for at least two reasons:

  1. It is inconsistent with literally every other implementation I've reviewed (~20 at this point).
  2. It does not match the behavior of route-recognizer, which we also bundle.

This means that our behavior does not work on hard refresh and also triggers strange serialization inconsistencies when run through a string or object path. I'm writing a full blog post about query strings which will be published this week which will detail a path forward.

I'm of the opinion that we should not attempt patches to this behavior without solving the underlying problem as we will simply introduce new and different failure modes. Until we have a complete solution (which I'll write as part of landing the new route-recognizer) we should ride out our existing known behavior and new reports should be closed as a duplicate of this bug.

@pixelhandler
Copy link
Contributor

pixelhandler commented Sep 9, 2016

@nathanhammond sounds like you have a plan forward for the solution as part of the route-recognizer work, nice. Will you post that write up as an RFC?

@webark
Copy link
Contributor

webark commented Oct 6, 2016

@nathanhammond do you have a link to that blog post?

@nathanhammond
Copy link
Member Author

@cwarny
Copy link

cwarny commented Feb 9, 2017

Any update on this? My use case is an array query param getting stringified. Any elegant workaround in the meantime?

@sarupbanskota
Copy link

sarupbanskota commented Jul 5, 2017

@cwarny Have you tried initializing the property corresponding to the query param as A()?

e.g

// within controller
tags: A(),
queryParams: ['tags']

@robclancy
Copy link

Is this ever going to be fixed?

@robclancy
Copy link

robclancy commented Apr 30, 2018

I'm just using this mixin now...

import Mixin from '@ember/object/mixin';
import { typeOf } from '@ember/utils';

function basicArray(array) {
  if (typeOf(array) !== 'array') {
    return false;
  }

  return array.every((value) => {
    return ['string', 'number'].includes(typeOf(value));
  });
}

export default Mixin.create({
  serializeQueryParam(value, urlKey, defaultValueType) {
    if (defaultValueType === 'array' && basicArray(value)) {
      return value.slice();
    }

    return this._super(value, urlKey, defaultValueType);
  },
  deserializeQueryParam(value, urlKey, defaultValueType) {
    if (defaultValueType === 'array' && basicArray(value)) {
      return value.slice();
    }

    return this._super(value, urlKey, defaultValueType);
  },
});

@pixelhandler
Copy link
Contributor

@acorncom @cwarny @johnbradley @nathanhammond @robclancy @rwjblue @sarupbanskota @webark is this still an issue, perhaps we should close or create a new reproduction of this, what do you think?

@webark
Copy link
Contributor

webark commented Sep 29, 2018

This was created in kind of wrapping up a number of other query param serialization bugs. I believe if those come up again, we can address them one at a time.

@robclancy
Copy link

I commented out my mixin and it is still happening on version 3.1.3. I can upgrade to test more but I haven't noticed anything in change logs about this being touched.

@tpetrone
Copy link

tpetrone commented Nov 26, 2018

I am experiencing something similar.
Url is being serialised as:
http://localhost:4200/v1/companies?filter[manufacturer-profile__manufacturer-quality-certificate-ids.all]=["5bf9a3f0141edcd078a620b2"]&filter[manufacturer-profile__manufacturer-service-ids.all][]=5bf9a3ef141edcd078a62086&page[number]=1&page[size]=10
instead of:
http://localhost:4200/v1/companies?filter[manufacturer-profile__manufacturer-quality-certificate-ids.all][]=5bf9a3f0141edcd078a620b2&filter[manufacturer-profile__manufacturer-service-ids.all][]=5bf9a3ef141edcd078a62086&page[number]=1&page[size]=10

I also can confirm the mixing suggested by @robclancy fixes the problem

@mattmarcum
Copy link

I saw this bug today on ember 3.5. I had two query-params, one a string and one an array. I think if the query-params got updated before the model refresh had completed then this bug would get triggered. Adding the above mixin to my route fixed it, also debouncing some query-param updates fixed it too.

@lynnfaraday
Copy link

Seeing some issues deserializing query params in 3.21. May be this same issue.

Query params are arrays defined with refresh: true.

Set the query params to foo: [ "A", "B" ], bar: ["C", "D"] via a transitionToRoute call and everything looks fine.

Leave the route and return, and now the query params are serialized JSON strings: foo: "[ \"A\", \"B\" ]", bar: "[ \"C\", \"D\" ]".

The mixin provided by robclancy above did provide a workaround.

I was unable to reproduce on a clean install of 3.21 so I'm guessing it's a race condition related to the route load timing.

rreckonerr added a commit to rreckonerr/ember.js that referenced this issue Oct 31, 2020
...on activeTransition in `_doTransition`.
`_processActiveTransitionQueryParams` returns serialized values and we serialize them again in `_prepareQueryParams`, which expects deserialized values.

Refs: emberjs#14174
@rreckonerr
Copy link
Contributor

Can you please check if #19236 fixes the issue?

@lucasmerat
Copy link

lucasmerat commented Feb 16, 2023

I appear to be having the same issue that @lynnfaraday had above.

It seems that the transition.from.queryParams array params in the model hook (used with transitionTo to return to a route with the existing query params) are in the format "[ \"A\", \"B\" ]"

Perhaps the issue is not fixed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.