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

Fix lookup_value_regex handling of non string types #1317

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lotvall
Copy link

@lotvall lotvall commented Oct 21, 2024

I was playing around with a very naive fix for #1316

However I did run into some issues:

Breaking regression tests:

def test_path_parameter_priority_matching(no_warnings, path_func, path_str, pattern, parameter_types):

And this test looks like it would be affected as well as it strips the UUID information:

reverse_transforms=[
lambda x: x.replace('format: uuid', 'pattern: ^[0-9]+$'),
lambda x: x.replace('\n description: A UUID string identifying this root.', '')
]

I would be happy to proceed with this PR and make the solution robust, but any early feedback on the issue and advice on the regressions would be appreciated before proceeding!

@lotvall lotvall force-pushed the integer-lookup-field-with-lookup-value-regex branch 2 times, most recently from 96a0a14 to 3911c55 Compare October 21, 2024 18:29
@lotvall lotvall marked this pull request as draft October 21, 2024 19:43
@lotvall lotvall force-pushed the integer-lookup-field-with-lookup-value-regex branch from 3911c55 to 1c25019 Compare October 22, 2024 09:25
@lotvall lotvall force-pushed the integer-lookup-field-with-lookup-value-regex branch from 1c25019 to da20fdb Compare October 22, 2024 17:14
Comment on lines -91 to -94
reverse_transforms=[
lambda x: x.replace('format: uuid', 'pattern: ^[0-9]+$'),
lambda x: x.replace('\n description: A UUID string identifying this root.', '')
]
Copy link
Author

Choose a reason for hiding this comment

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

I didn't proceed with this, but it would be interesting here to keep this information for UUID's as well, as it would expose more correct information to Open API.

  /root/{id}/:
    get:
      operationId: root_retrieve
      parameters:
      - in: path
        name: id
        schema:
          type: string
          format: uuid
        description: A UUID string identifying this root.
        required: true

Another approach that's adds pattern without removing format and description, but would be good to know if it's spec compliant:

        schema:
          type: string
          format: uuid
          pattern: [0-9]+
        description: A UUID string identifying this root.

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.

1 participant