Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

Work Item List Pagination links does not contain valid QueryParameters #427

Closed
aslakknutsen opened this issue Nov 3, 2016 · 0 comments
Closed
Assignees
Milestone

Comments

@aslakknutsen
Copy link
Contributor

The URLs provided by the Work Item List use ',' as Query Parameter separator instead of '&'

Expected behavior

links":{
  "first":"/api/workitems.2?page[offset]=0&page[limit]=0",
  "last":"/api/workitems.2?page[offset]=132067&page[limit]=1",
  "prev":"/api/workitems.2?page[offset]=132067&page[limit]=1"
}

Actual behavior

links":{
  "first":"/api/workitems.2?page[offset]=0,page[limit]=0",
  "last":"/api/workitems.2?page[offset]=132067,page[limit]=1",
  "prev":"/api/workitems.2?page[offset]=132067,page[limit]=1"
}

Steps to reproduce the problem

Invoke the service.

The 'quick' fix is to just replace ',' with '&', but due to Go JSON encoders built in HTMLEscape it converts all < > & to e.g. \u0026. While fully spec compliant for JSON strings, JS does not seem to understand much of it without a manual string replace.

In Go 1.6 I've only managed to 'fix' this by a rather elaborate workaround:

  • Make links in goa defined as d.Any (interface{})
  • Make all links Required (to avoid a pointer to an interface{} being generated)
  • Make a custom string type with a custom MarshalJSON function
  • Change the JSON encoder used from Go to the one defined in Goa

The downside of this approach, besides the elaborate swing dance, is that when Attributes are Required in Goa no omitempty JSON tag is generated for it, which means that 'nil' links will be serialized as "next": null. Not the end of the world but...

The reason for the HTMLEscape is due to some old browsers didn't decode the JSON properly when used in tags. While probably a valid reason, I don't see it as much of a reasonable usecase for us today.

The cleaner fix is to update to Go 1.7 where the JSON Encoder API now supports "func (enc *Encoder) SetEscapeHTML(on bool)". This would still require registration of custom encoders to override the default, but that's a minor task.

Depends on #426
Related #329

@aslakknutsen aslakknutsen added this to the Sprint #122 milestone Nov 3, 2016
@aslakknutsen aslakknutsen self-assigned this Nov 3, 2016
aslakknutsen added a commit to aslakknutsen/fabric8-wit that referenced this issue Nov 3, 2016
Query parameter separator should be '&' not ','

Upgrade to go 1.7 is required so we can add a new custom encoder to goa
that turn of & to \u0026 escaping in json strings

Fixes fabric8-services#427
Related fabric8-services#329
aslakknutsen added a commit to aslakknutsen/fabric8-wit that referenced this issue Nov 4, 2016
Query parameter separator should be '&' not ','

Upgrade to go 1.7 is required so we can add a new custom encoder to goa
that turn of & to \u0026 escaping in json strings

Fixes fabric8-services#427
Related fabric8-services#329
aslakknutsen added a commit to aslakknutsen/fabric8-wit that referenced this issue Nov 4, 2016
Query parameter separator should be '&' not ','

Upgrade to go 1.7 is required so we can add a new custom encoder to goa
that turn of & to \u0026 escaping in json strings

Fixes fabric8-services#427
Related fabric8-services#329
aslakknutsen added a commit to aslakknutsen/fabric8-wit that referenced this issue Nov 4, 2016
Query parameter separator should be '&' not ','

Upgrade to go 1.7 is required so we can add a new custom encoder to goa
that turn of & to \u0026 escaping in json strings

Fixes fabric8-services#427
Related fabric8-services#329
aslakknutsen added a commit to aslakknutsen/fabric8-wit that referenced this issue Nov 4, 2016
Query parameter separator should be '&' not ','

Upgrade to go 1.7 is required so we can add a new custom encoder to goa
that turn of & to \u0026 escaping in json strings

Fixes fabric8-services#427
Related fabric8-services#329
aslakknutsen added a commit to aslakknutsen/fabric8-wit that referenced this issue Nov 4, 2016
Query parameter separator should be '&' not ','

Upgrade to go 1.7 is required so we can add a new custom encoder to goa
that turn of & to \u0026 escaping in json strings

Fixes fabric8-services#427
Related fabric8-services#329
aslakknutsen added a commit that referenced this issue Nov 4, 2016
Query parameter separator should be '&' not ','

Upgrade to go 1.7 is required so we can add a new custom encoder to goa
that turn of & to \u0026 escaping in json strings

Fixes #427
Related #329
aslakknutsen added a commit to aslakknutsen/fabric8-wit that referenced this issue Nov 7, 2016
* Decoding the values on a global scale cause decoding errors in clients
* AngularJS handles the decoding fine without it

Reverts parts of fabric8-services#427 (unescape of & < >)
aslakknutsen added a commit to aslakknutsen/fabric8-wit that referenced this issue Nov 7, 2016
* Decoding the values on a global scale cause decoding errors in clients
* AngularJS handles the decoding fine without it

Reverts parts of fabric8-services#427 (unescape of & < >)
aslakknutsen added a commit that referenced this issue Nov 7, 2016
* Decoding the values on a global scale cause decoding errors in clients
* AngularJS handles the decoding fine without it

Reverts parts of #427 (unescape of & < >)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant