Disallow extra parameters or fields in queries

[alyssadai]

Context

Now that we have POST /datasets and POST /subjects, the request payloads for the two endpoints differ in only one field: dataset_uuids (used by /subjects but not by /datasets).

At present, extra fields in the JSON request body are silently ignored, meaning you can feasibly send a request like:

{
  "min_age": 50,
  "max_age": 70,
  "dataset_uuids": ["http://neurobagel.org/vocab/ds001", "http://neurobagel.org/vocab/ds002"]
}

to the /datasets endpoint and get results, giving the impression that the provided dataset_uuids filter actually had an effect.
Since this behaviour can be misleading (and also makes the request body harder to test), one option is to disallow fields that aren't explicitly defined the request body model.

One downside with disallowing extra parameters is that removing or renaming existing params becomes a more "visibly" breaking change. e.g., right now, if we were to rename a query field and someone continued using the old name, it would appear as if the query still worked since results would still be returned (even though the filter itself would no longer have an effect).

Implementation

Can use:

    model_config = ConfigDict(extra="forbid")

Concrete problem right now

• hard to differentiate the request bodies between datasets endpoint and subjects endpoint
  • makes it hard to test for "invalid" requests that should result
  • makes it hard to notice bugs when we send incorrect requests to the API
• a user may send a subjects request body to datasets endpoint assuming from the silence that datasets endpoint can filter
  • also typos or misspellings of fields would not be detected

Decision

• We will disallow additional parameters

Consequences:

• Old nodes will break for queries that involve new, deprecated, or misspelled request parameters
• These nodes will continue to function normally for requests that do not include the unsupported request parameters

TODO:

• Forbid extra query parameters
• (f-API / query tool): ensure that the federated response model can handle responses from different nodes that may have different shapes (i.e., if some nodes have a new parameter and others are outdated)

[alyssadai]

Any thoughts @neurobagel/dev?

[surchs]

Quick dump of my thoughts:

• Good (i.e. yes, make extra fields not allowed)
  • avoid ambiguity for developer / API consumer
  • helpful / informative validation error for typos / impossible states (e.g. min_age = 60 and invented_max_age = 55)
  • probably easier to catch bugs, probably also security benefits
• Not so good (i.e. case against making it mandatory)
  • technically this would itself be a breaking change. i.e. if we "require" APIs to strictly validate and build tooling that expects this
  • more importantly: it makes "adding" new fields a breaking change when normally they would not be. so practically, every time we introduce a new field (even optional) field in the request body and upgrade a f-API to use this field, any node that lags behind the most recent latest will start refusing to accept queries / throw errors.
  • This is going to massively increase the cost of API changes for us, because we now always have to
    • weigh any update against the cost of asking all nodes to upgrade
    • make sure the entire network upgrades at the same time, or accept phases where large parts of the network are down
    • ask and support all of our node maintainers in doing timely upgrades
  • At a minimum, this will make us more hesitant / reluctant to iterate the API quickly, slowing down overall development
  • I would also say that it will cost us a bunch of goodwill with our node maintainers, because each request to upgrade will likely only result in marginal UX improvements, so seem more like a nuisance
  • there is generally a principle of being "permissive" with input and strict with output for APIs. Probably shouldn't follow this as gospel, but it shapes expectations of people working with our tools, and so I wouldn't count on folks being thrilled to have to do frequent upgrades or drop off completely

So bottom line: I think the benefits don't outweigh the costs here. Since the original issue was one of "how can we differentiate /datasets from /subjects in a request", I would suggest we focus on this problem (differentiation) instead. I'm sure we can find other solutions to differentiate them.

[alyssadai]

Thanks @surchs. My other arguments on advantages of disallowing fields would be:

• Each node can have a clear signal when their n-API is out of date (as well as it being apparent to us), preventing nodes from lagging behind and causing unexpected behaviours with the rest of the network
• Allowing n-APIs to silently ignore newly introduced query parameters means that federated queries may have incorrect/unreliable results, since only some nodes might be returning actual matches (that actually filter based on the new parameter)

[surchs]

Decision after discussion: we will disallow additional parameters. This will make old nodes break for queries that involve new (or misspelled) request parameters. However: these nodes will continue to function normally for requests that do not include the new (or misspelled) request parameters.

Functionally / for a user, this will mean that a query that includes a new request parameter will only include results from nodes that already support this new parameter - and this is likely what we want. The cost / friction of introducing new parameters for us will therefore be more limited than what I said above, because the network will continue to function in most cases - and we will quickly see which nodes are trailing behind the rest and can show them why it'd be worth to upgrade