API: hosting integrations endpoint versioning/structure

[humitos]

Review initial JSON response to keep consistency with APIv3 resources. It uses a small modified version of the APIv3 serializers for known resources, removing some URL fields that can't be resolved from El Proxito.

Sharing the same serializers that our APIv3 make our response a lot more consitent between the endpoints and allow us to expand the behavior by adding more fields in a structured way.

Besides, these changes include a minimal checking of the version coming from X-RTD-Hosting-Integrations-Version header to decide which JSON structure return, allowing us to support multiple version at the same time in case we require to do some breaking changes.

The JavaScript client is already sending this header in all of its requests. See readthedocs/addons#28

Closes #10211

[agjohnson]

I feel like we may have had similar conversations in the past, but are we using any versioning patterns like this anywhere else? I think we maybe talked about what this looks like for APIv3 or our configuration file in the past

[humitos]

@agjohnson yeah, we talked about this pattern multiple times. We are using "something similar" in EmbedAPIv3 where we are also getting a HTTP header with the doctool and the version of it. However, we have only v1 response there for now and we haven't started performing the check yet.

This time, I want to start from start with the versioning so it's pretty clear what structure belongs to what version from the beginning. We can always change the implementation in the future if we want to, but making usage of the HTTP header from the start is great.

[agjohnson]

Yeah agreed that versioning is a great place to start from. On the implementation side, I could see how maybe a class based approach could eventually work better, especially if we anticipate changing parts of the response slightly.

For example if we wanted to alter projects response slightly in v1, I could see this pattern helping:

class AddonsV0(AddonsBase):
    def get_projects(self):
        # Return v0 projects structure

class AddonsV1(AddonsBase):
    def get_projects(self):
        # Return v1 projects structure

This helps avoid rewriting everything else in that data structure.

Just my immediate thought though, it certainly can shift over time and doesn't need to start this mutable of a structure.

[humitos]

That pattern looks good. I understand the idea would be to have "one method per main key" in the JSON, right?

[humitos]

I just wanted to note that this will add a non-trivial overhead from the start, if we want to go in that direction. Maybe this can be a later refactor?

[agjohnson]

I understand the idea would be to have "one method per main key" in the JSON, right?

Something like that, probably similar to Django model views or similar.

Maybe this can be a later refactor?

Yup, absolutely, it's not a now thing. Just raised here in case we immediately anticipate problems maintaining one method per response structure. It's not even a pattern to work about until after we have users relying on the API response.

[agjohnson]

With the client -> addons conversation, there are some naming changes here, but otherwise this is looking great.

Most of my notes are just future ideas, the only immediate change I see is that we likely want the version name in the flyout versions structure. All the other pieces are later pieces, after we have some features public and usable.

[agjohnson]

I like this pattern. It also helps push the translations into the application which I like. Translations will be A Thing though, so we can punt on that conversation either way.

[humitos]

Well, this is just a hard coded/default example.

In theory, what I understood from the work that @stsewd did, is that the author of the docs is whom will decide the names and the values of the filters. So, we need to keep talking how that data will be passed (see readthedocs/addons#13)

[agjohnson]

This is probably commentary on the build serializer and not this PR, but I found myself wanting some helpers here around the build state data. The data is all here, but I had to replicate a good deal of backend logic to determine things like is_finished, is_cancelled, is_active, etc.

Consider this a side request I supposed 🙂

[agjohnson]

For example, I had to sort through a lot of build logic with the API response this the build detail view, for example: https://github.com/readthedocs/ext-theme/blob/main/src/js/build/detail.js#L236-L283

[humitos]

The data is all here, but I had to replicate a good deal of backend logic to determine things like is_finished, is_cancelled, is_active, etc.

This is a pretty common discussion between backend and frontend teams 😄 . IMHO, the data is there and it's the FE who should create "the helpers" required to present the data as they need --but I guess this is kind of a philosophical discussion. We can continue talking about this outside this PR.

In any case, if we decide changing anything here, it should be done in the BuildSerializer.

[agjohnson]

project.language.name here is an example of where we could support a translated API response, probably based on accept headers. If the addition of a language aware API endpoint changes the implementation here drastically, maybe this is a now conversation. But I'm assuming it's not and we can come back to this as we give users more access into this response structure.

[agjohnson]

I'm guessing this is also probably helpful to not point users of this response structure towards resources on APIv3, where they will likely need to use an authenticated API client.

[humitos]

You have a good point here, yes.

However, there are other URLs that El Proxito is not able to resolve currently;

• project's dashboard page
• dashboard build's detail page
• downloads
• etc

So, we still need to find a way to resolve those and/or hard code them if we think that's fine for now.

[agjohnson]

👍

[humitos]

the only immediate change I see is that we likely want the version name in the flyout versions structure

I haven't touched the structure of features because we are not going to use it for the initial test. In any case, any Project, Version or Build object that's gonna be rendered inside features will use the same serializer we are using at the root keys. So, they will have all those fields as well.

I didn't change those "just yet" because it will also require a change in the client to keep working. We can coordinate both changes in a following up PR if it's not urgent.

[agjohnson]

That makes sense. Maybe a todo on that response code would be a good addition then. We can return when we want to break our own usage of the API 😄

[humitos]

I'm thinking a little more ahead in time here and just thought that we could have better top keys as well. Mainly for project, version and build. I think it will be helpful to group all the required objects under a more general key. This is the proposal:

{
  "projects": {
    "current": { ... },
    "translations": [{ ... }],  // list of all translations for this project
    ...
  },
  "versions": {
    "current": { ... },
    "active": [{ ... }],  // list of all active versions for this project
    ...
  },
  "builds": {
    "current": { ... },
    ...
  }
}

This will allow us to expand these objects in the future by returning more categorized results. "Active versions" and "Translations" are the most clear here, since they will be used for the flyout and with the current structure from this PR I feel we will end up having version and active_versions at the same level, where I think that versions.current and versions.active is clearer and a nicer expandable structure.

Also, since this is "common data" that's not specifically tied to a particular "addon" itself --because it's re-usable data, I think it makes more to be at a top level key than under features.flyout.versions and features.flyout.translations as I originally wrote. @agjohnson @ericholscher what do you think?

[agjohnson]

I think it's a solid improvement. I'd agree that the version instances don't need to be nested in the addon configuration section, which feels more like it should be user data and defaults. I could see us referencing versions from an addons.flyout.version_ids: [1, 2, 3] though

[humitos]

@agjohnson

I could see us referencing versions from an addons.flyout.version_ids: [1, 2, 3] though

I'm not sure to follow this part. Can you expand why we would need these version ids on the flyout? What they will be useful for here?

[agjohnson]

I could see this being useful if we want to include a complete list of versions in the top level of the data response, and only want to reference a few versions for the flyout.

We might want to return all active versions at the top level listing (including hidden versions and/or pull request versions), but the version selector only shows active, non-pull request versions.

[humitos]

We might want to return all active versions at the top level listing (including hidden versions and/or pull request versions), but the version selector only shows active, non-pull request versions.

In that case, we should have versions.active, versions.hidden and versions.external. That would be a lot more clear, and will avoid "the linking versions" for the flyout; IMO, that only adds unnecessary complexity.

In fact, this was my whole point when suggesting this new structure with another level: "We can add more filtered/categorized data for the top keys in the future without breaking the structure"

[agjohnson]

In that case, we should have versions.active, versions.hidden and versions.external

Thinking about this pattern more, it actually seems fairly complex, most especially from the user/author perspective though. I'd probably lean back towards a single list of versions instead multiple lists of versions.

One problem I see with this approach is that we/users/authors will have to iterate over multiple lists of versions to look up a single project/version/etc. This will also be confusing if projects/versions can exist in more than one listing.

I think it will be helpful to group all the required objects under a more general key.

Perhaps to step back, what use case do you see here? Perhaps it's not clear why you think this could be helpful to us/users/authors.

That would be a lot more clear, and will avoid "the linking versions" for the flyout

I mentioned an explicit list of version ids that we expect in the flyout because without that list, each user/theme author/etc needs to have a fair amount of understanding about our models to accurately show a list of projects even. That is, the version must be active, not a PR version, not hidden, not private, and the reader needs to have access to the version. Unless we want dozens of non-exclusive lists of versions, a lookup field like addons.flyout.version_ids: [1, 2, 3] is explicit and easy to work with by comparison.

[humitos]

I think we both want something similar but we are getting confused because of the naming used to explain this. It seems that versions.active is not a good name in the context I've used, because it also implies those versions are not hidden, are not PRs and the user has access to them (even if they are private ones).

The main point here is to have a extendable structure in the future when we need to add more values. Have the ability to put "all Project objects" under a top key projects; and the same for Version and Build. We can figure it out the naming of those sub-keys later if we want to.

I'll move forward with the structure <object>.current and later we can decide the other names.

[humitos]

BTW, we cannot return "all the versions" because that will destroy our database. So, there is going to be always an implicit filter. I tried to make those filter explicit by adding a the name of the filter in the sub-key, but it seems that they weren't 100% accurated.

[agjohnson]

Have the ability to put "all Project objects" under a top key projects; and the same for Version and Build

Agreed, I like this part of the pattern you outlined.

I'll move forward with the structure object.current and later we can decide the other names.

I also like this part of the pattern 👍

It was just sublists like version.active or version.hidden that seemed complex to me

BTW, we cannot return "all the versions" because that will destroy our database. So, there is going to be always an implicit filter.

I think I understand this and probably agree here. But to clarify, I wouldn't return inactive versions or versions we don't expect to use in display of the version selector/etc. This is where users/authors should have to use our API directly.

We can figure it out the naming of those sub-keys later if we want to.

Also agreed here, we can come back to this decision/discussion later. My issue was probably mostly with building out the API too much to start, we only really need a single list of projects to continue immediately.

[humitos]

I wouldn't return inactive versions or versions we don't expect to use in display of the version selector/etc. This is where users/authors should have to use our API directly.

Yes. I gave versions.active as example because those are the ones we are currently shown in the flyout and I think we will keep the same behavior in the new client.

However, that versions.active also implies:

• only versions the current user hitting the API has permissions on
• exclude hidden versions
• exclude external/PR versions

and I think there was where the confusion came. It's not "just active=True versions" -- and that's why I said the naming was properly not 100% accurate.

In any case, we can move forward with the latest changes. However, I didn't get an approve 🤷🏼