Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenAPI schemas #15

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

traines-source
Copy link

  • fixes pretty param to be a query param
  • adds missing from/to params for /journeys
  • adds some response schemas

disclaimer:

  • even fields marked as required or not nullable might be missing or null under unknown circumstances
  • some fields not marked as required might always be present
  • some fields present in some responses are missing from the schema
    --> don't rely on it, just use it as a quick way to generate a client

@derhuerst
Copy link
Member

When we chatted before, I forgot to tell you that there are community-maintained TypeScript typings, sorry. These may (have been) used to generate the JSON Schema definitions.

@@ -147,8 +147,284 @@ Works like \`/stops/{id}/departures\`, except that it uses [\`hafasClient.arriva
content: {
'application/json': {
schema: {
type: 'array',
items: {type: 'object'}, // todo
'type': 'array',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about moving the each "root" schema into a variable, to reduce the nesting level?

'tripId': {
'type': 'string'
},
'stop': {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move re-used sub schemas (e.g. a stop) into a variable each, and then use them here.

@traines-source
Copy link
Author

Auto-generating the schemas from the TS typings instead of from some JSON instances indeed seems better. There is only one drawback I have noticed so far: It doesn't distinguish between null and not present. But most libraries/programming languages don't do that anyways, so it shouldn't really be a problem.

Thinking about it again, we should have one common schema file for all endpoints. That way, it's easier to replace it with a new auto-generated version. And the reused sub-schemas (using "$ref") actually get reused and turn into the same class in the generated code in the client. I will have to take a more thorough look at this.

@derhuerst
Copy link
Member

Thinking about it again, we should have one common schema file for all endpoints. That way, it's easier to replace it with a new auto-generated version. And the reused sub-schemas (using "$ref") actually get reused and turn into the same class in the generated code in the client. I will have to take a more thorough look at this.

We can host the schemas in the hafas-rest-api#4 branch, and then re-use them in bvg-rest#6, db-rest#6, hvv-rest#6 & vbb-rest#6. What do you think?

@traines-source
Copy link
Author

In case that didn't become clear, I was talking about the endpoints like journeys, departures, arrivals, etc., which in the first iteration have completely separate schemas. In a generated client, that would lead to separate classes of the same structure (e.g. for a stop).
I had hoped that the schemas propagate from hafas-rest-api to db-rest etc., but I haven't tested it.

@derhuerst
Copy link
Member

In case that didn't become clear, I was talking about the endpoints like journeys, departures, arrivals, etc., which in the first iteration have completely separate schemas.

Yes, we can refactor common parts into common (sub-)schemas later.

I had hoped that the schemas propagate from hafas-rest-api to db-rest etc., but I haven't tested it.

They do, but some *-rest projects have a slightly customised response, given that they use a hafas-client profile that modifies the data parsed from HAFAS.

- use one central schema to reuse ($ref) definitions between endpoints, so that only one class per type is generated in clients
- generate schema file from @types/hafas-client using script
@traines-source
Copy link
Author

I have changed the approach now.

  • use one central schema to reuse ($ref) definitions between endpoints, so that only one class per type is generated in clients
  • generate schema file from @types/hafas-client using script (using JSONSchema lib, some alterations for compatibility with OpenAPI necessary)
  • no fields marked as required

@derhuerst
Copy link
Member

derhuerst commented Dec 3, 2022

I finally found time and energy to look at this from a broader perspective. Thank you for waiting.

Also, I'd like to cc @bergmannjg, who has contributed the hafas-client typings, and generally uses and knows about typings a lot more in this context. Do you have an opinion on how we should derive the OpenAPI/JSON Schema definitions from a hafas-client?

Some problems that are not yet solved by this PR:

profiles

hafas-client provides a common API to query several HAFAS endpoints; It also provides endpoint-specific customisations called "profiles", each with

  • a custom list of products
  • minor (but relevant) tweaks to hafas-client's interface.

For example, the hafas-client/p/db profile adds an opt.firstClass: boolean flag to journeys() (input), and a loadFactor field to Arrival & Departure (output).

As the enum of supported products is quite relevant to people using hafas-rest-api-based APIs, and as profiles might actually alter hafas-client's interface in a backwards-incompatible way, it is important to generate custom OpenAPI/JSON Schema definitions for each hafas-rest-api-consuming project.

@bergmannjg As a first step towards this, do you think it would be possible to let @types/hafas-client/p/*/index.d.ts define and export a modified/extended TypeScript schema definition that not only describes what the respective profile looks like, but also what the respective createClient(profile) will look like? For example, @types/hafas-client/p/db/index.d.ts would export a HafasClientWithDbProfile interface with radar() and without lines(). I don't know the TypeScript tooling well enough to answer if this is feasible.

As the next step, we could modify scripts/generate-schema.js from this PR to

  1. take either the path to a TypeScript schema definition (preferred) or the name of a hafas-client profile (less flexible) as an argument, and
  2. generate the profile-specific OpenAPI definitions this way.

Dependents of hafas-rest-api (e.g. db-rest) would have to call generate-schema.js and and use the generated definitions when building the API server, just like how they do it with the Markdown API already (1, 2).

passing the generated defintions into hafas-rest-api

This follows from the requirement to generate hafas-rest-api-consumer-specific OpenAPI definitions (see above): The generated schema should be stored as a file in the consumer's directory, and passed into hafas-rest-api's createApi(), e.g. as openApiComponents.

Then, all default routes (those implemented by hafas-rest-api), as well as those defined by the consumer, can access the HAFAS-endpoint-specific OpenAPI definitions.

I'm hestitant to pass a "full"/root OpenAPI spec into createApi() though, as

  • it is not complete anyways, as the routes are missing, and
  • it seems more cumbersome to customize than a plain components object.

What do you think?

excluding createClient.*

Currently, the generated OpenAPI defintions contain (almost) every type/interface twice, as Foo and createClient.Foo. This is because the TypeScript typings (understandably) also describe createClient() & createClient.Profile, and apparently because typescript-json-schema generates re-used types/interfaces twice?

This is a minor issue, as it is only about noise/bloat being displayed in the OpenAPI GUIs and possibly generated in not-so-clever client generators, but nevertheless I think we should tackle it eventually™️.

related: YousefED/typescript-json-schema#282

@derhuerst
Copy link
Member

Another unrelated note:
@traines-source Since you opened the PR, I have updated master to contain the v6 implementation of the API(s), doing some backwards-incompatible changes and upgrading to hafas-client@6. You can either

  • keep this PR against the v5 code base and let me port it to v6 (in this case, pick 5 as this PR's target branch), or
  • adapt this PR to the v6 changes.

The former option is probably less work for you, the latter less work for me. Up to you!

@bergmannjg
Copy link

@derhuerst: a first note to your questions

Do you have an opinion on how we should derive the OpenAPI/JSON Schema definitions from a hafas-client?

Please have look at hafas-api-tsoa. It uses tsoa as an alternative to typescript-json-schema. The api spec is generated from the TS typings and from decorators in the controller file.

It is important to generate custom OpenAPI/JSON Schema definitions for each hafas-rest-api consumer.

Usng tsoa the custom definitions may be generated from different controllers.

@derhuerst
Copy link
Member

derhuerst commented Dec 5, 2022

I'm a bit hesitant to use tools like tsoa because they impose a very specific structure upon your API, which IMHO is neither more understandable (the controller pattern really doesn't fit to what hafas-rest-api does) nor more extendable.

@derhuerst
Copy link
Member

@bergmannjg As a first step towards this, do you think it would be possible to let @types/hafas-client/p/*/index.d.ts define and export a modified/extended TypeScript schema definition that not only describes what the respective profile looks like, but also what the respective createClient(profile) will look like? For example, @types/hafas-client/p/db/index.d.ts would export a HafasClientWithDbProfile interface with radar() and without lines(). I don't know the TypeScript tooling well enough to answer if this is feasible.

Currently, @types/hafas-client describes an interface that is only given if I use hafas-client/p/db (not with e.g. hafas-client/p/nvv). Where shall I open an Issue so we can discuss this further? I would like to avoid (domain-specific) spam in the (very general-purpose) DefinitelyTyped repo.

@bergmannjg
Copy link

This branch contains code to check and create the typing file, here is a short description.

Currently, @types/hafas-client describes an interface that is only given if I use hafas-client/p/db (not with e.g. hafas-client/p/nvv)

Can you give an example, the types should work with all profiles.

@traines-source
Copy link
Author

Some thoughts:

  • There was something going on why I needed both createClient.Foo and Foo, but I don't remember why, have to look at it again. Or maybe it was just that I was happy that it worked at all after having tried other JSON schema libs that didn't do what we need.
  • I personally like the tsoa/in-code-annotation approach a lot, but I'm not sure it is easily applicable to the hafas-client codebase, that's probably what you meant @derhuerst. (But it saves you from the ugly string manipulation that I had to do to convert JSON schema to OpenAPI schema because I haven't found a lib that actually does that directly.)
  • I feel that it is not absolutely necessary to take into account the different profiles (but I might not know them well enough). E.g. AFAIK the products are currently just represented as an arbitrary key/value object or string and loadFactor is just an optional field in the TS typings. For me, the OpenAPI spec is a "best-effort" thing that you might want to tweak to fit your needs anyways (anecdote: the lib that I'm using to create Golang types from the OpenAPI (2.0) spec generates nullable types for required fields and non-nullable types for non-required fields, with the justification that with required fields, it is important to know whether they were actively set, while for non-required fields the type default is fine, see issue. For fields like delay where there is an important difference between null and 0, I needed it the other way round and thus ironically I set it to required...)
  • I already noticed some of the changes in v6 recently, should not be too much work I guess. But that is something that should be adjusted in the TS typings first.

@derhuerst
Copy link
Member

derhuerst commented Dec 11, 2022

  • I personally like the tsoa/in-code-annotation approach a lot, but I'm not sure it is easily applicable to the hafas-client codebase, that's probably what you meant @derhuerst. (But it saves you from the ugly string manipulation that I had to do to convert JSON schema to OpenAPI schema because I haven't found a lib that actually does that directly.)

Yes, indeed it prevents the (somewhat hacky and potentially incomplete) manipulation of the generated JSON schema, at a high cost in other area: Structuring all of the code according to the controller pattern, which IMO does not fit well to hafas-client's "RPC nature" as well as hafas-rest-api's dynamic-ness.

  • I feel that it is not absolutely necessary to take into account the different profiles (but I might not know them well enough). […] For me, the OpenAPI spec is a "best-effort" thing that you might want to tweak to fit your needs anyways […].

Maybe I misunderstood you, but there are at least two reasons why I'm not a fan of this approach which both come down to long-term maintenance:

  • With a) so many projects in general and b) so many stacked on top of hafas-client, I try to reduce both manual maintenance and human error caused by it to a minimum. This means that manually tweaking a schema (or double-checking a diff) after regenerating it is too error-prone.
    • (I'm aware that static typing can help with this to a certain degree, but so far, I don't see a way in which it doesn't get in the way of hafas-client's (and all dependent libs') highly dynamic nature: Letting its profiles override arbitrary parts of the createClient() API. Maybe there is a way, see port to a different language, e.g. Rust? hafas-client#238 for more discussion.)
  • People get in touch with my rather often with questions about *.transport.rest. While that is fine – In fact, I really appreciate that they don't just ignore the issue and/or give up, but help me see broken and unintuitive aspects! –, I'd like to keep the amount of questions caused by unnecessarily unintuitive behaviour of the APIs.
  • I already noticed some of the changes in v6 recently, should not be too much work I guess. But that is something that should be adjusted in the TS typings first.

Great! Let's first discuss the steps forward here though, so that you don't waste time working on things that we might revise anyways.

@bergmannjg
Copy link

it is important to generate custom OpenAPI/JSON Schema definitions for each hafas-rest-api consumer

It may be possible to generate profile specific typings from the general @types/hafas-client/index.d.ts file using
the corresponding products.js and index.js files. These typings can be the input of generate-schema.js.

For example: change the type of mode

    mode: 'train' | 'bus' | 'watercraft' | 'taxi' | 'gondola' | 'aircraft' | 'car' | 'bicycle' | 'walking';
}

with

    mode: 'train' | 'bus' | 'taxi';
}

for the nvv profile using the products array.

The flags of the profile (like trip or radar) can be used to adapt hafas-client's interface methods.

Maybe there should be flags for db profile specific fields firstclass, loadfactor, etc.

The conversions can be implemented with a shell script or node program.

excluding createClient.*

This should be solved with ES modules. The createClient namespace doesn't exist anymore.

@traines-source
Copy link
Author

With a) so many projects in general and b) so many stacked on top of hafas-client, I try to reduce both manual maintenance and human error caused by it to a minimum. This means that manually tweaking a schema (or double-checking a diff) after regenerating it is too error-prone.

With manually tweaking the schema, I meant the end user who uses it to generate the classes/types in whatever programming language they are using. Talking about schema generation in hafas-rest-api, I think it's indeed a good idea to do it on the fly at runtime (the only drawback there that we have to add the TS typings as a runtime dependency). I can have a look at that when we have agreed on how to proceed and as soon as the TS typings have been updated to match v6.

Since there still doesn't seem to be anything that really differs structurally, I'm still not convinced we should adapt the schema to profiles. This can even have the drawback for end users that they can not use the same (generated) code base for different HAFAS APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants