[validation] add error if path do not start with a slash #997
Conversation
|
Looks useful. Is there some reason why we can't fix this upstream in swagger-parser, however? |
No, I like the idea. I did not think about it. Lets add this now in OpenAPI-Generator, file an issue and PR in Swagger-Parser. If it is accepted there, we can revert this change when we do the next Swagger-Parser Update in our code. |
The fact that this doesn't exist in swagger-parser means that adding it could potentially break many consumers. I'd be surprised if swagger-parser accepted this before a major release version. |
|
@jmini I've labeled this as breaking change without fallback because any users with invalid paths objects in their spec will no longer be able to generate code. This could happen if one has created a custom generator which resolves paths and automatically prefixes them, or if the user has written custom template specific to their invalid spec (in which case, our recommendation to add the preceding slash will invalidate the custom templates). Is your intention for this to be a hard error, and therefore a breaking change? (edit: referring to Validate.java) Is there reasoning that:
|
|
@jimschubert thank you for this valuable feedback.
For me, it more means that nobody has brought the case before. From what I see, this lib is more issue-driven (stuff is improved when someone submit an issue and a PR). I am not sure that they decided on purpose to support a feature or not or in this case to report an error or not. Checking for the rule "Paths must begin with a In fact the kind of error message they report, are more technical (the parser was failing somewhere) than for an input not being conform to the OpenAPI Specification. I am more interested by the way OpenAPI-Generator should react in case of an input to being invalid, like this one: openapi: '3.0.1'
info:
title: ping test
version: '1.0'
servers:
- url: 'http://localhost:8000/api'
paths:
ping/me:
get:
operationId: pingGet
responses:
'201':
description: OKI did not check for each generator provided by the project, but it is easy to find at least some of them that are generating something that do not work out of this input. I have proposed to add a validation, because I had the impression that this is the pattern you recommended here #251 (comment):
Now if I understood you correctly, in this specific case you would prefer a solution where we correct the spec by our self treating the path as it was named No problem for me to rewrite the PR in this direction. I do not really have a strong intention or opinion for this PR and I am open for better solutions. Please confirm first that I understood your message correctly. |
|
@jmini I'm sorry if my previous comment was confusing. To clarify, I would think this should be implemented similarly to the issue you've linked:
|
I have opened #1034 for this specific part (is not a breaking change, can be added to |
|
@jmini regarding this from my previous comment:
Do you think we should have this as part of this PR? I think releasing the change without this could be frustrating for any users who don't know they've written or consumed an invalid spec. |
|
@wing328 I don't know if you've had a chance to look at this. What are your thoughts on merging it into master now? I had added a breaking change label, but upon further consideration I think it's more of a bug (we're missing a spec validation) rather than a breaking change (generation will fail for people with invalid specs). Basically, Jérémie's changes in #1034 will automatically fix the missing slash issue, and this PR will add this is a validation error. This will only be a breaking change for people with invalid specs. Our current approach is to validate the spec on generation, and the expectation should be that generation only succeeds by default for valid specs. We do allow users to generate by skipping spec validation, so although to default behavior changes for those with invalid specs they should not be blocked from generation. Here's examples of the change: Default behaviorGenerate with --skip-validate-specNote, if you compile and test on this branch, you'll get this in However, if you merge current master into this branch before evaluation, you'll have what I consider to be the "correct" path: So I think we can replace the breaking change label with a bug label and merge. I just didn't want to do this without a second pair of eyes, in case it caused confusion. |
|
Closing this as I implemented it locally only to realize that swagger-parser also results in an error now. |
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\.master,3.3.x,4.0.x. Default:master.Description of the PR
From #967 (comment)
This spec is not valid:
Because the path
ping/medo not conform to this rule:See the OpenAPI Specification 3.0.1.
Some editors like Swagger online editor or the KaiZen-Editor are reporting errors, but Swagger-Parser do not.
This PR add additional errors to the Swagger-Parser error message. If the user decide to by-pass the validation he might get invalid generated code.