NYCPlanning / ae-zoning-api

This application is API for serving data related to zoning and tax lots.
1 stars 0 forks source link

Add support for exploded array query parameters #193

Closed TangoYankee closed 3 months ago

TangoYankee commented 4 months ago

Description

There appears to be no standardized way to format, and subsequently parse, arrays in query parameters. Over several years, languages and frameworks like PHP and Ruby on Rails have spun their own implementations of formatting arrays. The ecosystem appears to have coalesced around three primary formats:

1) delimited strings

Each of these three primary approaches works well for different tooling. Approach 1 and 2 two are supported well by OpenAPI. Approach 3 is supported well by Axios and QS.

In an OpenAPI GitHub thread from 2014, a user highlights that Array query parameters were not supported. The user was specifically interested in using format 3. The OpenApi maintainers responded by adding explicit support for formats 1 and 2. At the time (v2), this was the collectionFormat property. It has since been refactored in Open API v3 to style: form with type:array. Format 1 is denoted with explode:false. Format 2 is denoted with explode:true (and is the default value). Despite Format 3 being the originally requested format, support was never added with descriptive parameters. Instead, users are expected to make the brackets part of the variable name, ie) param[]. Having brackets in the name itself causes compatibility issues with our frameworks, which are not configured to recognize brackets as "separate" from the rest of the variable name.

Moving from documentation to implementation, we are relying primarily on axios to format requests and qs to parse them. The Tan Stack libraries rely on axios. Next relies on express, which sets qs as its default query parser. qs decodes Format 3 reliably because the bracket notation always tells qs that the data should be an array. However, it struggles with Formats 1 and 2. Format 1 does not indicate whether it is an array or a string which happens to contain commas and is instead parsed as a string. (It is possible to pass a comma=true option but it is difficult to automatically apply that to only the array parameters). Qs handles format 2 well, when there are at least two elements. When there is only one element, format 2 is indistinguishable from format 1 and its problems. axios has its own formatter. It defaults to Format 3 but has a paramserializer indexes option that when set to null will make the urls follow Format 2. It is possible to pass a completely custom serializer to support Format 1. However, that means overriding all of axios' serializing.

There isn't a clear overlap in the formats well supported by both the documentation and the implementation tools. As the API is implemented, we support Format 1. We achieve this by checking the json schema for the observed parameter and splitting on the comma when the parameter is documented as a array. Changing completely to Format 3 would mean that we could remove this manual split because qs/express/nest would automatically recognize and parse it as an array. However, this would require placing the brackets directly in the parameter name, which would cause other errors parsing the variable. Format 2 does not truly serve as a compromise between 1 and 3 because it has all the same problems as 1 when there is only element. Overriding the axios to use format 1 creates a lot of risk for exposing us to circumstances that are unforeseen to us but covered by the axios implementation.

Moving forward, we should prefer and document Format 1. It is the more succinct format and it lends itself well to being documented. However, we should also support Format 3. It is much simpler to implement support for Format 3 in the server than it is to reconfigure client requests to use Format 1. The server merely needs to check whether qs already parsed the data into an array already and then skip the splitting step. We can also add a note that arrays also support format 3 when using tools like axios.

Acceptance criteria

TylerMatteo commented 3 months ago

Approved your PR and thanks for the write up. I'm very tempted to say we should only support option 3 as it seems to be a de facto standard for encoding arrays in OpenAPI 3.0, and it would mean we could always assuming string params that may happen to contain commas are always just strings. However, the tooling ecosystem being what it is, I'm confident this approach of supporting 1 and 3 is the right way to go for now. It looks like Axios/QS on the front end and Express/QS on the backend do support 3, but Kubb doesn't, at least not in the way we would want. You can just set name: lons[] in the openapi.yaml and Kubb will regenerate the code, but it names the property literally "lons[]" in the generated code, which is yukky.