🎨 Enhance Response Schemas, reorganise common properties, and add operationIds to methods #2661
Conversation
| `get_players_by_account_id_select_matches` for the path: "/players/{account_id}/matches". | ||
| */ | ||
|
|
||
| function generateOperationId(method, path) { |
There was a problem hiding this comment.
What's the purpose of this function? Isn't the path string identifying a specific function to call anyway?
There was a problem hiding this comment.
At the moment, the spec has no OperationIds for the different method.
This just results in warnings when using the OpenAPI spec in a generator, because the generator has to compile its own OperationIds. But, different versions of OpenAPI generator can then give different naming conventions, so it's good to add operationIds for consistency.
This function just provides a generic operationId for any method/path. I used regex to add this function on every route, so we now have generic operationIds by default, like:
`get_players_by_account_id` for the path: "/players/{account_id}"
`get_players_by_account_id_select_matches` for the path: "/players/{account_id}/matches".
*/
Now if anyone wants improved names for any given route, then the operationId can be edited with a string, to replace this function call.
There was a problem hiding this comment.
@howardchung For example, spec.js now has:
"/players/{account_id}/histograms/{field}": {
get: {
operationId: generateOperationId(
"get",
"/players/{account_id}/histograms/{field}"
),The name of the path is passed to the function to generate the operation id.
The resulting spec.json is then:
"/players/{account_id}/histograms/{field}": {
"get": {
"operationId": "get_players_by_account_id_histograms_by_field",There was a problem hiding this comment.
I guess what I'm asking is what the operation ID is used for. If it's just a unique identifier, then doesn't the route itself work? Unless you aren't allowed to have { or / characters
There was a problem hiding this comment.
openapi-generator can generate a client from the spec.json to interface with the API. So it can give you HeroesAPI.getHeroes(). That name getHeroes can be automatically inferred from the route itself. But it can also get very messy for long routes, plus naming convention can be inconsistent across different languages or version. So, an operationId standardizes that.
The function I wrote takes the route as input to generate a method name for it. It's almost identical to the names that openapi-generator would have given anyway, just more consistent with long route names. So, now you can control how one wants to name it. The operationId field can be edited to hardcode a preferred name if wanted.
|
Doing some more refactoring. We have responses/commonProperties and requests/commonProperties, so breaking that up a bit more for maintainability |
|
@howardchung by the way, I notice there's some unused request query/parameters being defined: withAccountIdParam: {
name: "with_account_id",
in: "query",
description: "Account IDs on the player's team (array)",
required: false,
schema: {
type: "integer",
},
},
againstAccountIdParam: {
name: "against_account_id",
in: "query",
description: "Account IDs against the player's team (array)",
required: false,
schema: {
type: "integer",
},
},
...
minMmrParam: {
name: "min_mmr",
in: "query",
description: "Minimum average MMR",
required: false,
schema: {
type: "integer",
},
},
maxMmrParam: {
name: "max_mmr",
in: "query",
description: "Maximum average MMR",
required: false,
schema: {
type: "integer",
},
},
minTimeParam: {
name: "min_time",
in: "query",
description: "Minimum start time (Unix time)",
required: false,
schema: {
type: "integer",
},
},
maxTimeParam: {
name: "max_time",
in: "query",
description: "Maximum start time (Unix time)",
required: false,
schema: {
type: "integer",
},
},
matchOverviewParam: {
name: "overview",
in: "query",
description: "Only fetch data required for match overview page",
required: false,
schema: {
type: "integer",
},
},But they are not used anywhere. Before I remove them, are these query params intended to be used anywhere? Maybe it's meant to be part of Do we still want to implement these, or can I remove it? |
|
Ok, pretty happy with it now, I think the schemas + params are much better organised, in folders Still more improvements possible, like parameterising all the response properties. Currently only a few common params in For example BenchmarksResponse defines the following pattern 7 times: So it'll help a lot if everything is parameterised. It'll deduplicate and help spot inconsistencies. Lastly it'll also help to move the route func definitions out from |
…e response object
|
^ last change just made hero_id a required field in |
|
Unused parameters--if they aren't in the API code then feel free to delete them |
Sorted, thanks. Some of the unused queries make a lot of sense to add - like min/maxMmrParam, and min/maxDuration. Perhaps it was intended for match filtering? Is it worth making an issue/PR to start adding that option? Also, let me know if this broke anything, here or discord. Anything I'm responsible for breaking I feel responsible to fix 😆 |
|
I suspect it was meant to filter matches at some point (but only for pro/public game samples in SQL, not the cassandra-backed player match list) |
Some response properties need to be updated to reflect that they can be nullable.
There is a ton of duplication throughout the spec, so this PR starts the initiative of defining common properties in a single location (renaming the previously defined
properties.jsmodule tocommonProperties.js-- it was not being used yet and "properties" is too generic of a name).Now if a field turns out to nullable, it can be edited in 1 place.
List of changes:
RecordsResponsereturns strings for: match_id, start_time, hero_id, and score. All of these should be integers, to be standardised with all other usages. So, 💥 breaking changeRecordsResponsenow returns integers instead of stringsPlayerHeroesResponsereturns hero_id: string, when consistency would require an int. Server response type is updated.TeamPlayersResponsereturns account_id: string. This is now consistently an int.HeroStatsResponsedidn't line up with the actual API response, so I updated the response params ✅commonPropertiesmodulesresponses/propertiesandrequests/queryParamsconvertSpec.jsscript added, to generate spec.json, which is now also tracked for version control.Decided to refactor.
routes/responses.jscontained all of the response objects. Now it's triaged intoroutes/responses/:(thank you GPT-4 for helping me auto-generate these file names)
Discussion for further refactoring is welcome