Spec changes for OpenAPI generator

Created by: creckord

These commits contain various changes to make the spec work better with the OpenAPI generator.

They're mostly adding naming and some cleanup and should not cause any noticable changes to the REST API.

Created by: creckord

I noticed a couple of other inconsistencies, mainly concerning the relationship between Listing and ListingVersion and how it is expressed in the REST API.

In some places, we treat ListingVersion as a sub-resource of Listing, e.g. /installs/{listing_id}/{version_id}, while in others, we treat it as a "standalone" resource, e.g. /listing_versions/{version_id} (instead of /listings/{listing_id}/{version_id}).

I'll add some comments inline in the spec.

Created by creckord

Review: Commented

Can't comment inline on this, but the licence_type parameter in the GET /listings endpoint seems off.

On the one hand, its type is given as a scalar string, on the other hand, it has style: form and explode: true as if it is supposed to be an array.

Do we want to allow to query for multiple (or'ed) values for one field? In that case this should probably be an array. Otherwise we should probably get rid of style and explode here.

And if we allow to search for multiple values, should we do this for all/most other parameters as well?

Created by: creckord

Do we want /listing_versions or /listing/<id>/versions? To me, a version only makes sense as a part of a listing, so I would expect it to be a sub-resource.

Created by: creckord

Also, where would I post a new version to, and how would I associate it with its listing?

Created by: creckord

What is the difference between /installs/{listing_id} and /installs/{listing_id}/metrics? They have different filtering options but return the same schema, so they look like they're just different "flavours" of the same endpoint?

Created by: creckord

Having <listing_id>/<version_id> here seems inconsistent with the /listing_versions/<id> scheme above. Also, if version_id is supposed to be globally unique, there wouldn't be a need to specify both ids here?

For the association, there is a listing_id FK that makes the association for us. This information is included when you request a listing, but is written separately so that you don't need to post the entire listing to delete a version. The URL doesn't make an enormous difference in the end, it's just far cleaner to have it be a separate path. I can talk to Chris to see if that makes sense on our end

The reason for splitting is to avoid having to search through multiple tables to know if something is a listing or a version. By having separate methods, it means that users can drill down into analytics based on a version or a listing without a slow down of determining type.

As to consistency, this is a drilldown of a single type of data, where listing_versions support a new entity type in the data.

The fact that these return the same data was an error on my part, apologies. I'll create an issue to track that change. /installs/{id} returns a simple count of installs for a given listing. This call can be more accurate than the general listing count which will be aggregated on a schedule rather than an on-demand count which is a heavy performance call. This can also have filters to represent how many installs were tracked for the given filter set, allowing some drilldown analytics of a given listing.

/installs/{listing_id}/metrics returns the overall metrics for a given yearly period and uses previously aggregated data to make the response much faster than on-demand queries.

It seems like this component is missing from your PR. Is it a staged change that wasn't pushed maybe?

Can't comment inline on this, but the licence_type parameter in the GET /listings endpoint seems off.

On the one hand, its type is given as a scalar string, on the other hand, it has style: form and explode: true as if it is supposed to be an array.

Do we want to allow to query for multiple (or'ed) values for one field? In that case this should probably be an array. Otherwise we should probably get rid of style and explode here.

And if we allow to search for multiple values, should we do this for all/most other parameters as well?

Currently, we support single values for most fields. This field should instead be checked as a single rather than multiple for the moment. Do you believe there is value in adding multiple value support?

approved this merge request

I would be open to support both.