Required fields in HTTP API docs

[woylie]

Preflight checklist

• [X] I could not find a solution in the existing issues, docs, nor discussions.
• [X] I agree to follow this project's Code of Conduct.
• [X] I have read and am following this repository's Contribution Guidelines.
• [ ] This issue affects my Ory Cloud project.
• [X] I have joined the Ory Community Slack.
• [X] I am signed up to the Ory Security Patch Newsletter.

Describe the bug

There are some inconsistencies in the Hydra HTTP API documentation regarding required fields.

1. consentRequest.skip is optional, but loginRequest.skip is required.
2. consentRequest.subject and logoutRequest.subject are optional, but loginRequest.subject is required.
3. consentRequest.client and logoutRequest.client are optional, but loginRequest.client is required.

There may be more cases where the required attribute is missing in the documentation. Might be worth to check the type definitions.

Reproducing the bug

1. go to https://www.ory.sh/docs/hydra/reference/api
2. read

Relevant log output

No response

Relevant configuration

No response

Version

v1.11.7

On which operating system are you observing this issue?

No response

In which environment are you deploying?

No response

Additional Context

No response

[aeneasr]

Thank you for the report! For the required type, it's kind of a mixed bag. Basically it makes most sense for a field to be required when it is expected in a request - so e.g. as a request parameter or request body:

POST /foo?required=asdf

{
  "i-am-required": "true"
}

because there, the client is informed: yo, this field is needed if you call this API endpoint.

However, we also set required - in some cases - for responses. The problem here is that we reuse models for both requests and responses - for example when creating an OAuth2 Client or when fetching an OAuth2 client.

However, for responses, I don't think it's that useful to have required responses as all fields will be set always anyways, except for some advanced use cases where omitempty is set.

I'm not sure how to handle this consistently throughout the code base. Do you have ideas or experience with this?

/cc @vinckr because this might be helpful for context to you

[woylie]

The issue is that when you build your own Hydra client application, you see all these fields in the responses that are optional, and if you need one of them in your logic, now you must always handle the case where that field might not be set. If the response fields are defined as required, though, you can just make the response parser strict and be confident that the field will always be set in the rest of the code base.

If currently models are shared for requests and responses and the required fields differ, I guess the only way to handle this is to have separate SomethingRequest (or SomethingInput or SomethingCreateInput or SomethingCreateRequest) and SomethingResponse (or SomethingCreateResponse or SomethingOutput or SomethingCreateOutput) models.

Since the model will be the same across all responses, in the case of the oAuth2Client model we could have oAuth2Client for all responses and oAuth2ClientInput for create and put (I assume the required fields would be the same here).

[aeneasr]

I see! That makes sense. It’s actually funny because in Golang the generator works the other way around. Required fields are there generated as pointers (e.g. *string) which requires an extra dereference which can panic if the underlying value is nil. That’s the original reason why we set many values to required: false as it complicates the code we consume ourselves. But I guess other languages are handling this differently 🥲