Skip to content

Init implementation for the Open API validation#125

Closed
xuzhg wants to merge 1 commit into
microsoft:masterfrom
xuzhg:Validation
Closed

Init implementation for the Open API validation#125
xuzhg wants to merge 1 commit into
microsoft:masterfrom
xuzhg:Validation

Conversation

@xuzhg

@xuzhg xuzhg commented Dec 12, 2017

Copy link
Copy Markdown
Contributor

@PerthCharern @darrelmiller

I want to implement the validation logic for the Open API element. Here's my design (it's not finished).

General design:

  1. Can validation each part individually. for example, Developer create a object, he can call the validate api to validate the created object.

  2. Validate rule can be customized. For example, developer can create their own validation rules.

Detail:

  1. OpenApiElementValidator.cs has the public apis

  2. .\Validations\Visitors has the visitor classes for each element.

  3. .\Validations\Rules has the rule classes for each element.

Please have a look and give me some feedback.

/// <summary>
/// Extension name MUST start with "x-".
/// </summary>
private static readonly ValidationRule<IOpenApiExtensible> ExtensionNameMustStartWithXDollar =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

XDollar? Do you mean XDash?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes.

@darrelmiller

Copy link
Copy Markdown
Member

@xuzhg I understand why a developer should be able to create validation for extensions, however I'm not sure why we would want to be able to create custom validation for standard elements. Do you have any examples of this?

@xuzhg

xuzhg commented Dec 12, 2017

Copy link
Copy Markdown
Contributor Author

@darrelmiller I don't mean to create custom validation, I mean to create custom rule. For example, some customers want to make sure all the path item name meet a pattern, they can create their own rules to validate.

@darrelmiller

Copy link
Copy Markdown
Member

@xuzhg Creating rules that validate OpenAPI documents against some kind of business specific guidelines feels out of scope for the core library to me. Is there some specific stuff that you want to be able to do for OData that is driving this?

@xuzhg

xuzhg commented Dec 13, 2017

Copy link
Copy Markdown
Contributor Author

@darrelmiller

If you don't think the Core lib should have the ability to extend its functionality for the validation. We can just make the Validate(this I element, ValidatiaonRuleSet ruleset, out errors) as private method.

However, the whole rule/ruleset structural makes us easy to add the validation logic for the DOM classes.

@xuzhg

xuzhg commented Dec 13, 2017

Copy link
Copy Markdown
Contributor Author

@darrelmiller @PerthCharern I don't see It's in scope of Preview. Let's have a discussion after that.

@PerthCharern

Copy link
Copy Markdown
Contributor

Darrel, I don't think extending rules is out of scope per se. One purpose of the core + readers libs is to be the "base" for other things that can be built (e.g. more types of readers), and in this case, types of validation rules. Given that we make the "default rule set" public and with the overload Sam is providing for the entry point, I think we can allow our customers to build their own rules if needs be. They might want to restrict certain sets of strings (path strings, etc.) to enforce consistency just among their sets of APIs.


In reply to: 351419521 [](ancestors = 351419521)

@PerthCharern

Copy link
Copy Markdown
Contributor

I understand though that this extensibility is kind of a stretch (since extensions in other dimensions like the reader still adheres strictly to OpenAPI spec), but I don't see the harm of providing this potentiality.


In reply to: 351804977 [](ancestors = 351804977,351419521)

internal void Pop()
{
this._path.Pop();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we make _path a property and simply call Push/Pop on _path itself? I don't think we need to make these methods on the context.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@PerthCharern I make them as internal and the methods itself can let me coding less character. 😆

/// <returns>The error string.</returns>
public override string ToString()
{
return "ErroCode: " + ErrorCode + ", " + ErrorPath + " | " + ErrorMessage;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"ErroCode: " + ErrorCode + ", " + ErrorPath + " | " + ErrorMessage; [](start = 19, length = 67)

This is a little awkward. Maybe we don't need this?

@xuzhg xuzhg Dec 14, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Don't you think it's helpful for debug?

@xuzhg

xuzhg commented Dec 14, 2017

Copy link
Copy Markdown
Contributor Author

@PerthCharern Thanks! Ok, Let me continue on this PR (even though I think #133 is better. 😆 ).

@xuzhg

xuzhg commented Dec 15, 2017

Copy link
Copy Markdown
Contributor Author

@PerthCharern @darrelmiller I have to close this because the origin link failed.

I created a new PR to continue: #152

@xuzhg xuzhg closed this Dec 15, 2017
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