API source parameter validation improvements

[obulat]

Problem

The current source field validation splits the value by a comma and filters this list by removing items that are not in the valid source name list for the media type. Then, the serializer joins all of the valid source names by comma and returns this string.

So, when one of the items in the source list is invalid or misspelled, it is quietly dropped. Furthermore, the source value is ignored completely if there's a single misspelled value. So, https://api.openverse.engineering/v1/images/?source=flick ignores the source parameter and returns all of the images. This might be very confusing if the user has made a spelling mistake.

Code validating the source field:

openverse/api/api/serializers/media_serializers.py

Lines 651 to 658 in 520cd7a

	def validate_source_field(value):
	"""Check whether source is a valid source."""
	allowed_sources = list(search_controller.get_sources(media_type).keys())
	sources = value.lower().split(",")
	sources = [source for source in sources if source in allowed_sources]
	value = ",".join(sources)
	return value

Solutions

1. I think, we should return a 400 error if any of the source values is invalid, clearly showing which one it is, and also returning the list of valid sources. For instance, if the value is source=flick,stocksnap, instead of returning the images from the single valid source, "Stocksnap", we should return a 400 error saying something like "An invalid source "flick" in the source parameter. The list of valid sources for images: ....".
   @WordPress/openverse-api , do you agree this option is better than other alternatives?

2. We could also continue the current behavior if at least one of the sources is valid and return a 400 if the source parameter is present but contains no valid sources.

[zackkrida]

I agree with your proposed solution #1.

[AetherUnbound]

I also agree with the first proposed solution.

[krysal]

I support to switch API behavior to option 1 too ➕

[dhruvkb]

While I agree, in theory, with solution no. 1, I have to point out some things that are bothering me.

1. Backwards compatibility: This solution would break some clients that are relying on the response to be 200 OK regardless of bad input. Apps that work today, even with misspelled source names, will break so this has to be released alongside a major version bump of the API.

2. Future-proof validation: While it's also rare that we would remove a source, it would break any unmaintained clients that have the name of the source hardcoded. This can make it possible for us to unilaterally break apps when we have to drop a source.

I believe we should consider a more compromise-based solution where we continue to send 200 OK responses if there is at least one valid source in the list of sources (with warnings for the bad sources), switching to 400 Bad Request only if all sources listed are invalid (and thus the user are receiving an unfiltered response).

[obulat]

Future-proof validation: While it's also rare that we would remove a source, it would break any unmaintained clients that have the name of the source hardcoded. This can make it possible for us to unilaterally break apps when we have to drop a source.

This is a great point, I haven't thought about this!
I did think that we need to check if all of our official integrations use only valid sources.

Is there a way to add a detail to the response, something like "Invalid sources in request: source1, source2"? Is there a pattern of returning the API response while also adding details about the partially-incorrect request?

[dhruvkb]

I think we could add some field, like warnings (string[]), that are present alongside our regular response body of result_count, page_count, results etc. that can contain messages about validation issues. It will be an additional field in the response body so it will be backwards compatible.

I would have to search for how any other established APIs convey this pattern of 400-bad-request-but-salvageable-to-generate-a-reasonable-200-ok-response but the aforementioned field seems reasonable to me.

[zackkrida]

We also generally need to revisit how we handle API versioning, a flag I know @sarayourfriend has been waving for a long time. The idea that a change may require a version bump is something we need to be able handle moving forward. We need to do so thoughtfully, but we need space to be able to make and release breaking API changes.

[AetherUnbound]

Personally: I think it's okay to start issuing 400s for invalid input, even if we were issuing 200s. Part of API versioning (IMO) is defining the contract of the API responses and sticking to that. Our parameter set isn't changing here, just the set of valid inputs. In other words, if we view this as a "bug" rather than a behavior change, I think it's perfectly valid to just move to 400-ing this!

[sarayourfriend]

In other words, if we view this as a "bug" rather than a behavior change, I think it's perfectly valid to just move to 400-ing this!

That is a significant opinion on what the correct expectations of a versioned API are. In semver I'd consider this a minor version update, rather than a patch, because it breaks backwards compatibility. People famously depend on and sometimes program with a particular bug in mind. When a bug narrows the allowed inputs, it is necessarily breaking compatibility. Sometimes that's acceptable, others it may not be. For #3626 we've chosen to break compatibility, because we need to for security and performance longevity reasons. Similarly, we broke compatibility with certain ES query inputs in #3360 for the sake of performance and long term service stability.

In this case, we are narrowing the scope of "valid" inputs (or rather, inputs that will return a 200 rather than an error repsonse). That's sometimes an acceptable breakage for the same reasons I've mentioned above, for example, if the bad input could cause a data integrity issue. That isn't the case here though. The API potentially behaves strangely, but only because we haven't documented this "feature". If we updated the API docs to say "Source inputs that do not exactly match the name of a source defined in the stats endpoint for the media type are ignored" then the behaviour is no longer expected, and it does not cause long-term problems for our service, all while preventing the breakage of existing integrations that rely on this, whether intentionally (no need to sanitise bad user input) or unintentionally (they incorrectly capitalise the source name).

With software libraries we can log deprecation warnings to give users a heads up in a patch version before the minor version is released that causes the breakage, depending on the significance. To my knowledge, HTTP doesn't have any mechanism like that (no one is going to read a "x-deprecation-notes" header 🙊, maybe you could do a weird redirection kind of thing and re-write the parameter on the upstream location? not sure if HTTP even supports rewriting query params on redirects, user would still never know why it happened without detailed evaluation). We could send an email to registered apps who've sent authenticated requests in the last Y period of time, and accept that's a sufficient warning.

But... all of that is a philosophical and opinioned approach to versioning. We do not have a definition of what is meant or what can be reasonably expected by our version indication. To me, I'm of both minds that that means we can treat it as noise and do whatever we want, within reason; or that we can't do anything except additions or fixes that are necessary to ensure the stability and security of the service. In the latter approach, this particular change doesn't fit. But, yeah, that's just one view I can imagine us taking with respect to what that little v1/ prefix means, one of a diverse set of options.

My point being: we can't make statements like "this is or is not a breaking change we can accept" other than based on precedence, which thus far has been only breaking compatibility when it is necessary for long term stability and security, until we actually define what our API versioning entails and what users should expect. The fact is, there is a diverse range of possible expectations, and we haven't documented what the correct ones are, so almost any decision we make is meaningless, aside from trying to cause as minimal strictly unnecessary disruption as possible.

Anyway, just my view on this. If I gave my opinion about whether to make this change, or whether it's even a problem, I'd say two things:

1. Can we prove this is actually a problem affecting anyone making regular requests? If we query our request logs and aggregate all the sources ever passed to the query parameter, do we ever see any that aren't invalid? Or are we just finding an edge case in the code and imagining it is causing someone unknown difficulties? If in querying, we discover that no one is regularly sending invalid sources, then great, we can make the change and ignore the compatibility issue, because no significant number of requests actually ran into it anyway. New users won't ever know it was an issue, and very infrequent requesters won't remember it ever worked (or be surprised they were out of the loop so much that something like this seemed to work before but now doesn't).
2. What problem does this actually present our API? Is this actually a problem we need to solve, or just a funny edge case we can pretty reliably ignore because anyone significantly integrating with our API will notice that responses are not narrowed to the sources they've listed.

My bet is that there are invalid sources in requests (I sent one earlier today to the excluded_sources paramater by capitalising Flickr in a hand-written request, oops), but that they are either scarce or extremely common (like >80% of requests with multiple sources in the parameter) because of one or two commonly misspelled or erroneously handled sources. If they're scarce, then it doesn't matter what we do (point 1). If they aren't, then why do we need to solve it and break compatibility for all those requests? What problem does it pose our service in the near or long term?

My inclination is to do nothing, assuming I'm not missing something about the problem this edge case causes.

Anyway, my main point is, we are working in a limited definition of what our versioning means, and that presents difficulties in thinking through these problems, and the strategy thus far has been to avoid thinking about it at all when possible, and the work to move towards a concrete definition and explicit expectations is, like Zack said, something I've talked about for a while and we haven't got around to... it just hasn't been that important. On the other hand, we have conversations like this on a regular basis, and we also frequently run into issues with forgetting these considerations, like this thread did until you brought it up, @dhruvkb, or until it's too late and we've already added new stable features that we didn't get to verify worked the way we needed (collections path-based routes). There is precedence in all those converstaions, but no documentation, so if we aren't referencing precedence, we cannot make statements about what expectations are correct or incorrect about our versioning indication, only what is and is not absolutely necessary for our operational stability or what is possible based on whether it would actually affect anyone.

[sarayourfriend]

A third option is to do nothing 🙂

I'm not trying to be fasecious, but this either affects no requests at all (e.g., very very low number of requests with the parameter ever have invalid values) or it affects some to a lot of requests. In either case, we can do nothing, and it will cause no long-term problem for our service stability.

I've written more about my thought process in this comment: #3895 (reply in thread)

I'd ask: why is this a problem, who does it affect, and considering it is a breaking change, under what precedence or philosophy of versioning are we accepting that compatibility change, and is it a precedence we're willing to take forward for non-critical issues that pose no issue to operational stability or security? Or does it actually present such an issue? Then we need to document that and prioritise the change accordingly.

In any case, doing nothing is an option, unless there's a clear reason we need to break compatibility.

[dhruvkb]

+1 for first getting some tallies about the usage of this particular "feature" and only taking any decisions when we have some data on how many users it will affect and how.

[obulat]

I opened an issue for logging this: #3945

[obulat]

The logging changes have been deployed.

The query parameter we use for integration contains many sources that we've set hidden in the API. This is a typical warning:

[WARNING] [07ab0a87ac884d098995969448b64570] Invalid sources in search query: {'500px', 'eol', 'mccordmuseum', 'iha', 'thorvaldsensmuseum', 'behance', 'statensmuseum', 'worms', 'capl'}; sources query: 'wordpress,woc_tech,waltersartmuseum,thorvaldsensmuseum,statensmuseum,spacex,smithsonian_zoo_and_conservation,smithsonian_postal_museum,smithsonian_portrait_gallery,smithsonian_national_museum_of_natural_history,smithsonian_libraries,smithsonian_institution_archives,smithsonian_hirshhorn_museum,smithsonian_gardens,smithsonian_freer_gallery_of_art,smithsonian_cooper_hewitt_museum,smithsonian_anacostia_museum,smithsonian_american_indian_museum,smithsonian_american_history_museum,smithsonian_american_art_museum,smithsonian_air_and_space_museum,smithsonian_african_art_museum,smithsonian_african_american_history_museum,sciencemuseum,rijksmuseum,phylopic,nypl,nasa,museumsvictoria,met,mccordmuseum,iha,geographorguk,floraon,eol,digitaltmuseum,clevelandmuseum,brooklynmuseum,bio_diversity,behance,animaldiversity,WoRMS,CAPL,500px,rawpixel'

This means that all of the integrations requests will result in error responses, and we would have to be extremely careful when updating the existing sources and/or integrations request parameters if we decided to change the source parameter validations. Unless we implement #3421, which would centralize the included/excluded sources for integrations.

As a conclusion, I suppose "do nothing" suggested by @sarayourfriend is the only option now.

The main problem I see with the current behavior is when the user does not send any valid source parameters (i.e., something like "some_misspelled_name1,some_misspelledname_2") and then see results that they think are from those valid sources but are actually all results not filtered by source at all. Probably the best suggestion here, if we decide on the "do nothing" strategy, is to add documentation to the source parameter saying that if parameters are invalid, they are ignored.

[dhruvkb]

+1 for documenting the case of completely unfiltered response. For such cases, even a 400 Bad Request might be fine because the results will be wildly different from their expectation both in terms of quality and relevance as well as in terms of sensitivity and content safety. A common case might be people who misspell a museum slug and start seeing content from Flickr.

I did a bit more digging into my aforementioned suggestion of 200 OK with warnings in the response, and it seems like that is the most common way of handling cases where the input is partially acceptable and can generate a valid response. If at least one of the specified sources is valid, the response will only include that particular source and will be aligned to some extent with the expectation of the user and in such cases, 200 OK is the right status.

[sarayourfriend]

You reasoning for the 400 responses when there are no valid sources makes a lot of sense to me, Olga. So does the additional warnings key you're talking about Dhruv 👍

Here's an issue for it: #4030

I'll have a PR up momentarially.

[sarayourfriend]

Closed along with the completion of #4030