Skip to content
Snippets Groups Projects

Spec changes for OpenAPI generator

Merged Christopher Guindon requested to merge github/fork/creckord/mpc into master

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.

Merge request reports

Merged by Christopher GuindonChristopher Guindon 4 years ago (Jul 13, 2020 8:21pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • 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?

  • Christopher Guindon
181 191 get:
  • 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.

  • Christopher Guindon
  • 181 191 get:
  • Christopher Guindon
  • 264 278 get:
    • 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?

  • Christopher Guindon
  • 306 322 description: Unique ID of an individual listing
    307 323 required: true
    308 324 schema:
    309 type: string
    325 $ref: "#/components/schemas/ObjectID"
    310 326 - name: version_id
    • 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?

  • Ghost User
    Ghost User @ghost started a thread on commit eafe4561
  • 181 191 get:
    • Contributor

      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

  • Ghost User
    Ghost User @ghost started a thread on commit eafe4561
  • 306 322 description: Unique ID of an individual listing
    307 323 required: true
    308 324 schema:
    309 type: string
    325 $ref: "#/components/schemas/ObjectID"
    310 326 - name: version_id
    • Contributor

      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.

  • Ghost User
    Ghost User @ghost started a thread on commit eafe4561
  • 306 322 description: Unique ID of an individual listing
    307 323 required: true
    308 324 schema:
    309 type: string
    325 $ref: "#/components/schemas/ObjectID"
    310 326 - name: version_id
    • Contributor

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

  • Ghost User
    Ghost User @ghost started a thread on commit eafe4561
  • 264 278 get:
    • Contributor

      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.

  • Ghost User
    Ghost User @ghost started a thread on commit eafe4561
  • 86 82 This value should be in the format of `fieldname [ASC|DESC]`. For example, `installsrecent DESC`
    87 83 required: false
    88 84 schema:
    89 type: string
    85 $ref: "#/components/schemas/sort_whitelist"
    • Contributor

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

  • Contributor

    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?

  • Ghost User approved this merge request

    approved this merge request

  • Christopher Guindon
  • 181 191 get:
    Please register or sign in to reply
    Loading