openapi_parser icon indicating copy to clipboard operation
openapi_parser copied to clipboard

Added option to disable email format validation.

Open boof opened this issue 4 years ago • 3 comments

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

boof avatar Feb 25 '22 15:02 boof

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.

makdad avatar Mar 06 '22 04:03 makdad

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.

boof avatar Mar 11 '22 09:03 boof

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.

makdad avatar Mar 12 '22 08:03 makdad