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

feat: adds a name field to the server object #4469

Merged
merged 5 commits into from
Mar 27, 2025

Conversation

baywet
Copy link
Contributor

@baywet baywet commented Mar 19, 2025

fixes #1821

Tick one of the following options:

  • schema changes are included in this pull request
  • schema changes are needed for this pull request but not done yet
  • no schema changes are needed for this pull request

Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
@ralfhandl ralfhandl linked an issue Mar 19, 2025 that may be closed by this pull request
@ralfhandl
Copy link
Contributor

Could you please add a name to the first Server Object in the test case https://github.com/OAI/OpenAPI-Specification/blob/v3.2-dev/tests/schema/pass/servers.yaml?

(Adding it to the first one should avoid merge conflicts with #4456)

baywet and others added 3 commits March 19, 2025 12:16
Co-authored-by: Ralf Handl <ralf.handl@sap.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
@baywet
Copy link
Contributor Author

baywet commented Mar 19, 2025

Thanks for the quick review! I have made the requested updates.

@handrews
Copy link
Member

@baywet this makes name and description both unrestricted strings. Are there supposed to be limitations on what consistitutes a valid name? The issue mentions using these for code generation.

@ralfhandl ralfhandl requested a review from a team March 19, 2025 16:34
@baywet
Copy link
Contributor Author

baywet commented Mar 19, 2025

@handrews I was wondering that myself as I put the pull request together. So I went and looked for precedent work, like the parameter name, the license name, schema properties name, etc...
Although there are implicit limitations (parameters must abide by HTTP query parameters conventions if they are query parameters, path parameter conventions if they are path parameters, json schema properties must be a valid JSON property name, etc...), we don't call any of that explicitly out?

As for code generation, the tool can always run some kind of sanitation routine based on their own domain restrictions.

This is why I decided to leave it open, but happy to narrow it down based on this audience's feedback.

@handrews
Copy link
Member

@baywet thanks for the research and summary. If the TSC is fine with continuing in the same vein as existing name fields, I have no objection. We have had some complaints in this area (which I'm too lazy to dig up right now), but choosing the right restrictions is not an obvious thing. On the other hand, we should be a lot more clear in Moonwalk, particularly with full Unicode support being something people now expect.

@lornajane
Copy link
Contributor

I have no objections to either using the field name name, or to leaving the validation pretty open in common with the other fields. If I understood this correctly, the name is for tools outside of OpenAPI to use, we're not suggesting people can use this name to refer to a server entry from anywhere else within the OpenAPI file?

@baywet
Copy link
Contributor Author

baywet commented Mar 24, 2025

@lornajane correct, name is just informative as far as I understood the initial issue, no reference mechanism from the spec. It may be used in code generation or to produce other generated artifacts (docs, etc...) at which point we should probably leave that up to the tool consuming it.

@karenetheridge
Copy link
Member

Do we want to keep server objects as an array, rather than moving to an object now that we have a key to index each object by?

Right now there is an implied order in the objects (where the first one is attempted to be matched first), but we're not explicit about that in the spec, nor do we say anything about whether we should handle multiple entries matching. If only one may match, then order is irrelevant and we can move to an object.

@ralfhandl
Copy link
Contributor

Do we want to keep server objects as an array, rather than moving to an object now that we have a key to index each object by?

Yes, please! Adding an optional fixed field in a minor version is a non-breaking change that will not invalidate any existing OAD if its openapi field is bumped to 3.2.0.

Changing the structure of servers is something better done in a major version.

Assume you just want to use the improved tags, and now you have to change other completely unrelated things just to pass validation again - very annoying.

@miqui miqui merged commit 1a3ddd1 into OAI:v3.2-dev Mar 27, 2025
2 checks passed
@baywet baywet deleted the feat/server-name branch March 27, 2025 16:16
@karenetheridge
Copy link
Member

@ralfhandl yes I more meant that we might want to consider changing this in moonwalk :)

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

Successfully merging this pull request may close these issues.

Add 'name' field to servers
6 participants