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

Added option to disable email format validation. #129

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

Conversation

boof
Copy link

@boof boof commented Feb 25, 2022

Hello there,

we're currently facing the problem that we have to support RFC2822 addresses which are not covered by Ruby's URI email pattern.
Instead of removing the format from our API documentation we decided to go with this option to disable the format validation here. Another option would be to use the mail gem for example as its parser supports this standard.

What do you think?

Best
Florian @ M-Tribes

@makdad
Copy link
Contributor

makdad commented Mar 6, 2022

I see what this PR is attempting to do, and I generally understand it (in some ways, similar to #126) in that different implementors will have different interpretations of what format: email or format: date-time, etc., mean, based on their business requirements and implementations.

My first port-of-call in this case would be look at the specification itself to see if we can get any guidance.

In the case of date-time the specification clearly specifies RFC3339, so it seems to me that #126 is an improvement of this tool to match specification. However, email doesn't make the cut of defined specification, and instead:

Formats such as "email", "uuid", and so on, MAY be used even though undefined by this specification.

So from that perspective, I think we have a more generalized problem that we can solve here. I can imagine that any format not defined in the specification can be configured with a block (of course, with a supplied default for obvious things like email or uuid), but open to configuration by a power user that wants to further specify.

This would also allow (staying within the context of the OAS3 spec) for us to have functionality like custom formats, with custom validators.

Here's how I see this being implemented in a flexible fashion (this example ignores the current architecture of OpenAPI parser, I am just showing as an example of my thought):

# user-level configuration, I am imagining in the parser loading/config step
email_validator = Proc.new { |email| your_custom_rfc2822_validator(email) }

# `config` is configuration for how this Gem validates
config.set_custom_format_validator('email', email_validator)

a solution like this would not just solve the problem for email, but it would be true to the specification in that any user of the specification MAY define custom formats for their own purposes.

@boof
Copy link
Author

boof commented Mar 11, 2022

So you suggest making all the format validators configurable?

That wasn't my intention all along since I see only like email validating is quite challenging and often too restrictive. We're currently facing the issue having a proper documentation in place but tools like committee that depend on openapi parser report false positives due to this check being to restrictive.

Our first idea was to remove the format definition from the documentation but I challenged that decision as I don't see a reason to degrade quality of documentation because the the email validation giving false positives.

So instead I suggested fixing the validation but this is, as mentioned earlier, very hard to achieve. Even the mail gem can't make it.

Finally we came to the conclusion that we need to disable this check, and to make this optional I added this PR to be able to configure it.

@makdad
Copy link
Contributor

makdad commented Mar 12, 2022

I can see both ways working, actually. email is common enough, and is defined in the specification, so your solution makes sense. My point was that the specification actually allows for any value for format, meaning that we have an opportunity to solve your specific problem in a more general fashion.

However, my contributions to this repository are recent, and I don't necessarily have all of the answers on the maintainers' ideas for all architecture.

But you may have a good point that my suggestion might be "YAGNI". I have no intention to block the PR as-is, it merely inspired me for a more general-purpose solution that is more extensible, and includes the current problem solved by this PR.

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.

2 participants