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 uri_rfc3986? matcher to support validating full URL #94

Merged
merged 1 commit into from
Dec 27, 2021
Merged

Add uri_rfc3986? matcher to support validating full URL #94

merged 1 commit into from
Dec 27, 2021

Conversation

hieuk09
Copy link
Contributor

@hieuk09 hieuk09 commented Dec 22, 2021

I use uri? predicate to validate whether a string is an valid URI. However, this case fails:

schema = Dry::Schema.JSON do
  required(:picture_url).filled(:string, uri?: 'https')
end

schema.call(picture_url: '{https://picture.com/some_image.png}')
# => return successful, while it should be failed.

Upon reading code of URI module, I notice that URI::DEFAULT_PARSER.make_regexp creates a regex that matches only part of the string, and I have no way to avoid that behavior, aside from switching to URI::RFC3986_Parser::RFC3986_URI:

schema = Dry::Schema.JSON do
  required(:picture_url).filled(:string, format?: URI::RFC3986_Parser::RFC3986_URI)
end

However, this prevent me from using the nice error message that comes with uri? predicate. I suppose some people would have the same problems as me, so I would like to contribute it back to the repo.

I understand that changing uri? itself would be a breaking change, so I want to avoid that, and add a new uri_rfc3986? predicate instead. If people start using it over uri?, maybe we can consider deprecate uri? and replace it with uri_rfc3986? in v2.0.

@hieuk09 hieuk09 requested a review from solnic as a code owner December 22, 2021 11:55
@hieuk09
Copy link
Contributor Author

hieuk09 commented Dec 23, 2021

I see that there are some rubocop errors that are not related to the changes in this PR, should I fix them?

@flash-gordon
Copy link
Member

@hieuk09 I fixed them in master, pls rebase!

@hieuk09
Copy link
Contributor Author

hieuk09 commented Dec 24, 2021

Thank you, I rebased the branch.

@flash-gordon
Copy link
Member

@solnic thoughts?

Copy link
Member

@solnic solnic left a comment

Choose a reason for hiding this comment

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

Thanks. We can add it now and then think what to do with uri? in 2.0. Whatever behavior is least surprising is probably what we want to have in the end.

@solnic solnic merged commit e7676ed into dry-rb:master Dec 27, 2021
@hieuk09 hieuk09 deleted the bug/fix-match-invalid-uri branch December 27, 2021 08:40
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.

3 participants