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

Undeprecate comma separated expansion parameters #1696

Closed
tisto opened this issue Aug 25, 2023 · 8 comments
Closed

Undeprecate comma separated expansion parameters #1696

tisto opened this issue Aug 25, 2023 · 8 comments
Milestone

Comments

@tisto
Copy link
Sponsor Member

tisto commented Aug 25, 2023

Expanding multiple expansions at the same time used to be passed via a comma-separated list:

?expand=actions,breadcrumbs,navigation,workflow,types

The docs still show this actually:

https://plonerestapi.readthedocs.io/en/latest/usage/expansion.html?highlight=expand#expansion

Though, this was deprecated some time ago and instead one should use:

?expand:list=actions&expand:list=breadcrumbs&expand:list=navigation&expand:list=workflow&expand:list=types
@tisto tisto added this to the 9.x.x milestone Aug 25, 2023
@davisagli
Copy link
Sponsor Member

davisagli commented Aug 25, 2023

This is much less readable, and unfamiliar to anyone who doesn't know Zope. I would un-deprecate the old way and continue to support it.

@tisto
Copy link
Sponsor Member Author

tisto commented Aug 25, 2023

@davisagli I agree that the new recommended way is far less readable. However, we have to make sure that the way we pass a list via URL parameters is consistent. We can use the catalog way or come up with our own way. Though, it needs to be consistent. We decided some time ago that sticking with the zcatalog way would be the most pragmatic option we have.

Here is the PR where this was introduced:

#1299

cc @sneridagh

FYI: I don't consider this to be a blocker for a 9.0.0 release. It was just something that I found when checking for deprecation warnings in the p.restapi source code.

@davisagli
Copy link
Sponsor Member

I'm not sure what you mean by "the zcatalog way". Processsing parameters using suffixes like :list is a feature of the ZPublisher, not ZCatalog.

What makes it more pragmatic?

@tisto
Copy link
Sponsor Member Author

tisto commented Aug 25, 2023

@davisagli you are correct. This is how the zpublisher translates URL parameters to zcatalog queries.

This discussion is as old as plone.restapi. The question is: how do we encode complex data structures list nested dicts in a URL for a GET request (so that can be cached). We want a consistent way of doing so that works for simple lists (like for expand) as well as for more complex use cases (querystring-search, search endpoint).

My point is that we have to be consistent in how we pass lists or dicts or nested dicts in a URL. We need a way to do this on the backend with Python and on the frontend with JavaScript. Unfortunately, there is no recommended way how to do this. I did some research:

#1252

Lukas Graf did some research:

#45

and lately @robgietema as well if I am not mistaken. @sneridagh created the PR that deprecated the comma-separated way that we are currently discussing.

The pragmatic solution to this was, so far, that we just stick to the default way the ZPublisher handles this. This is definitely not the most elegant way, but at least we know what we are up to. The deprecation of the comma-separated way was our attempt to handle things consistently.

As said, I do not consider this to be a blocker for the 9 release. We can keep things as they are and just deprecate something for plone.restapi 10.

@davisagli
Copy link
Sponsor Member

I agree with the goal of consistency. But I don't think the ZPublisher approach handles everything we need -- particularly nested structures. So trying to use that as the consistent approach will not work, because then we'll still have to use something else when we need to serialize something it doesn't handle. For example we ended up implementing the GET version of @querystring-search using serialized JSON, not Zope parameter marshalling.

@tisto
Copy link
Sponsor Member Author

tisto commented Aug 25, 2023

@davisagli right, I forgot about this. This would mean, if we would use JSON URL encoding, it would look like this:

expand=foo%2Cbar%2Cbaz

Bottom line I guess is that we are horribly inconsistent in what we use right now...

For the sake of completeness, here is where we currently use ":list" in plone.restapi:

https://github.com/search?q=repo%3Aplone%2Fplone.restapi%20%3Alist&type=code

@davisagli
Copy link
Sponsor Member

We can consider using 2 different approaches and still call it consistent as long as we have clear rules on when to use each one.

Anyway, yeah, I'd say let's continue this discussion but not try to push it for plone.restapi 9

@tisto tisto changed the title Remove deprecated comma separated expansion parameters Undeprecate comma separated expansion parameters Sep 21, 2023
@davisagli
Copy link
Sponsor Member

Done in #1707

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

2 participants