Skip to content

Open-api: update prefix param description #9870

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Open-api: update prefix param description #9870

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion open-api/rest-catalog-open-api.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1444,7 +1444,7 @@ components:
schema:
type: string
required: true
description: An optional prefix in the path
description: Prefix in the path
Copy link
Member Author

Choose a reason for hiding this comment

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

It is actually a required field. Cannot be optional.

I tried changing the field to optional. I get validation error as "in: path" params must be required.
So updated the description.

Copy link
Contributor

Choose a reason for hiding this comment

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

so should required be set to false then?

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, if I set it to false (I tried changing the field to optional), validator will fail. So, I assumed Paths must be required and changed the description.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the error you're seeing?

The prefix is optional, but it depends on the definition. We can also update the default to be an empty string.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I change to required: false for prefix

Spec is invalid.
Issues:

	components.parameters.For path parameter prefix the required value should be true


Execution failed for task ':iceberg-open-api:validateRESTCatalogSpec'.
> Validation failed.

Copy link
Member Author

@ajantha-bhat ajantha-bhat Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @Fokko for digging more on this. Their goal for publishing V4 is end of 2024.

So, As of now (since spec is based on v3), it is a required field. So, lets update the description and merge this as it is confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jbonofre, @nastra: WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

personally I would just leave things as they are and not change anything at this point

Copy link
Contributor

Choose a reason for hiding this comment

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

So, As of now (since spec is based on v3), it is a required field. So, lets update the description and merge this as it is confusing.

I'm also in favor of leaving it like this. It is just yet another thing we cannot express in the open-API specification, but the intent is that the prefix is optional.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, let's keep as it is for now. As we have the new REST spec proposal, we can add a topic related to that.


table:
name: table
Expand Down