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

Querystring vocabulary ordering is not retained by REST API response #1854

Open
JeffersonBledsoe opened this issue Jan 10, 2025 · 5 comments

Comments

@JeffersonBledsoe
Copy link
Member

JeffersonBledsoe commented Jan 10, 2025

The sort_keys=True argument used for the response of all REST API calls means that any ordering which was present in the vocabulary is lost.

def render(self):
self.check_permission()
content = self.reply()
if content is not _no_content_marker:
self.request.response.setHeader("Content-Type", self.content_type)
return json.dumps(
content, indent=2, sort_keys=True, separators=(", ", ": ")
)

@ShailRT
Copy link

ShailRT commented Jan 10, 2025

so we only have to remove sort_keys argument?
@JeffersonBledsoe

@davisagli
Copy link
Member

davisagli commented Jan 10, 2025

@JeffersonBledsoe I'm not sure I understand the specific context where you're having trouble with the ordering -- are you talking about this endpoint? https://6.docs.plone.org/plone.restapi/docs/source/endpoints/vocabularies.html#get-a-vocabulary

If so then I doubt this is related to sort_keys=True, because that's for sorting object keys, but the vocabulary terms are in an items array, not an object.

In general there's no guarantee that the order of object properties will be preserved through JSON serialization[1], so I would almost consider this a feature rather than a bug, to make sure people don't accidentally try to depend on it being preserved. Where the order is important, we need to serialize that as an array rather than an object.

[1] See https://www.rfc-editor.org/rfc/rfc7159.html#page-6: "JSON parsing libraries have been observed to differ as to whether or not they make the ordering of object members visible to calling software. Implementations whose behavior does not depend on member ordering will be interoperable in the sense that they will not be affected by these differences."

@JeffersonBledsoe
Copy link
Member Author

JeffersonBledsoe commented Jan 10, 2025

@davisagli Thanks for the quick response! If you take a look at the querystring endpoint docs and look at the responses for something that has values in the value key (e.g. review state), you'll see the values are a dictionary with a key of 'title':

"values": {
                "external": {
                    "title": "Externally visible [external]"
                },
                "internal": {
                    "title": "Internal draft [internal]"
                },
                "internally_published": {
                    "title": "Internally published [internally_published]"
                },
                "pending": {
                    "title": "Pending [pending]"
                },
                "private": {
                    "title": "Private [private]"
                },
                "published": {
                    "title": "Published with accent \u00e9 [published]"
                },
                "rejected": {
                    "title": "Rejected [rejected]"
                },
                "spam": {
                    "title": "Spam [spam]"
                },
                "visible": {
                    "title": "Public draft [visible]"
                }
            }

I'm in the middle of a PR to plone.app.querystring (where the REST API fetches it's values from) to include an order key in the response so that even after serialization to JSON, the correct ordering can be re-achieved. Think of it the same as the 'blocks' and 'blocks_layout' keys.

In general there's no guarantee that the order of object properties will be preserved through JSON serialization, so I would almost consider this a feature rather than a bug, to make sure people don't accidentally try to depend on it being preserved. Where the order is important, we need to serialize that as an array rather than an object.

Yeah, I found this not long after posting this issue, I assumed JSON keys were ordered, but I stand corrected.

For a little context on how I came across this: We're trying to use collective.taxonomy in the Volto search block where the ordering is non-alphabetical

@davisagli
Copy link
Member

davisagli commented Jan 10, 2025

@JeffersonBledsoe Thanks. I was searching the querystring endpoint docs for "vocabulary" and forgot to look for "values". This is an example of why it's always helpful to show specific examples / steps to reproduce when you report an issue, and not just describe the problem. Anyway, I digress...

Yeah, this seems like a bug to me that the current serialization can't represent the order.

I'm in the middle of a PR to plone.app.querystring (where the REST API fetches it's values from) to include an order key in the response so that even after serialization to JSON, the correct ordering can be re-achieved. Think of it the same as the 'blocks' and 'blocks_layout' keys.

Okay. Other approaches we could consider:

  1. Change values to an array instead of an object.
  2. Add a new key (values_list?) that is an array, without changing the existing one.
    With either of these approaches, Volto would need to support both the old and new format to stay flexible with what version of plone.restapi is used. With the first option, we would need a new major version of plone.restapi since it's not backwards-compatible.

@JeffersonBledsoe
Copy link
Member Author

@JeffersonBledsoe Thanks. I was searching the querystring endpoint docs for "vocabulary" and forgot to look for "values". This is an example of why it's always helpful to show specific examples / steps to reproduce when you report an issue, and not just describe the problem. Anyway, I digress...

I completely agree, it was late when I filed this and I should have provided a better example, sorry for the drawn out discussion because of this!

Add a new key (values_list?)

Agreed on option 1 being breaking, which is why I'd like to avoid it. I did think about option 2, but the duplication of values felt like a waste of bytes and a potential tripping point for users of the endpoint (albeit a small one).p.a.querystring does currently only set title for the values, but I suppose this could have been extended by somebody at some point and so by having the order key, you still have access to these other attributes

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

No branches or pull requests

3 participants