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

Add additionalProperties to HttpLink, LocalizedString, ea #7

Open
wsalembi opened this issue Sep 6, 2023 · 3 comments
Open

Add additionalProperties to HttpLink, LocalizedString, ea #7

wsalembi opened this issue Sep 6, 2023 · 3 comments

Comments

@wsalembi
Copy link

wsalembi commented Sep 6, 2023

Following belgif/rest-guide#110 and the decision to not accept unknown properties, some objects in openapi-commons must be changed to mark them explicitly extensible

  • HttpLink: to add other fields next to href
  • LocalizedString: to add other languages
  • e.a ??
@pvdbosch
Copy link
Contributor

pvdbosch commented Sep 6, 2023

I don't think it is needed:

HttpLink

It is usually only used in the response, not in requests (and the rule only applies to requests).
Also, APIs usually use HttpLink in combination with an allOf, so the other properties are known, e.g.

PersonReference:
    allOf:
      - "$ref": "./common/v1/common-v1.yaml#/definitions/HttpLink"
      - type: object
        properties:
          ssin:
            "$ref": "#/definitions/Ssin"

LocalizedString

If a Belgian API deviates from the three declared languages (nl, fr, de), it usually declares its own variant type to be explicit about which languages are provided. E.g. if the server supports "nl", "fr" and "en", using the LocalizedString from openapi-common would lead the client to believe that "de" would be processed by the server but it would be silently ignored, and that "en" isn't supported.

Such a type can still evolve in a backwards-compatible manner. E.g. removal of a language is covered:

In specific situations, where a (known) input parameter is not needed anymore and can be safely ignored:

  • either it can stay in the API definition with a deprecation flag and a "not used anymore" description
  • or it can be removed from the API definition as long as the server ignores this specific parameter.

@wsalembi
Copy link
Author

wsalembi commented Sep 7, 2023

If types are meant to be extended then I think there is no issue in configuring that explicitly in the common schemas.

LocalizedString documents in the schema that additional languages can be added (#nl, fr, de are predefined, but additional languages can be added). I don't think it is necessary to subclass LocalizedString into LocalizedStringWithEnglishAdded to add en for example.

The same applies to HttpLink and also Problem. Problem is meant to be enriched with additional properties in the specification. For some problem types we want to make it very explicit by subclassing it, but it should not be an obligation.

We require a default /components/responses/ProblemResponse on each resource, but if that response is triggered we loose all the extra properties because we don't have added a catch-all additionalProperties map to the generated classes.

@pvdbosch
Copy link
Contributor

pvdbosch commented Sep 13, 2023

I think there are two closely related issues in this discussion: how to best document extensions of types, and how to implement open-world polymorphism. The latter issue is about the loss of extra properties when deserializing a Problem.

edit: moved rest of the comment to a new discussion in #153

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

No branches or pull requests

2 participants