-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[Slim] Upgrade API server integration tests to use Fake Petstore spec #354
Conversation
…-testing.yaml. Slim init and new models has been generated.
…oid PHP syntax errors.
… handles Fake Petstore spec much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good to me (but I've not yet tested it locally in my new machine)
cc @jebentier @dkarlovi @mandrean @jfastnacht @ackintosh (PHP Technical Committee) |
@wing328 and others
I didn't check this scenario, but I'm sure that codegen will produce broken stubs. Maybe this commit is a half measure and needs to be removed. |
Thanks for reviewing the fix and I see your concern. We'll look into a better fix if someone reports an issue with a spec. |
During testing with Swagger Editor. I've added $app->options('/{routes:.+}', function ($request, $response, $args) {
return $response;
});
$app->add(function ($req, $res, $next) {
$response = $next($req, $res);
return $response
->withHeader('Access-Control-Allow-Origin', 'http://editor.swagger.io')
->withHeader('Access-Control-Allow-Headers', 'X-Requested-With, Content-Type, Accept, Origin, Authorization, api_key')
->withHeader('Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE, PATCH, OPTIONS');
}); Without that headers you can't test samples with Swagger Editor. Test results:Pet
Store
User
fake_classname_tags 123#$%^
fake
$another-fake?
|
@ybelenko Thanks for the enhancement!
It may caused by the invalid json request body |
@ackintosh The same with:
Does this violate Swagger specification or any HTTP standards? Maybe we need to remove/change related endpoints, as soons as even Swagger Editor can't run it. |
@ybelenko Let's not blame anyone here as we've a blameless culture. If you really want someone to be "accountable", I'm don't mind taking the responsibility (and feel free to blame me offline via email or gitter)
0 (number) is a valid JSON if I'm not mistaken (as confirmed with https://jsonlint.com/). According to https://www.json.org/
One of these forms is
|
@wing328
If so, then I need to check new versions of Slim Framework. Error occures before any route implementation, maybe I need to create issue, that Well, JSON is pretty straightforward. What about GET request with body? For me it's anti-pattern and wrong implementation. I'm really wondering why this kind of endpoints don't throw any errors or warning in Swagger Editor. |
Looks like it's still "valid" to have a request body in a GET request: |
Oh..., sorry my misunderstand made you guys confused. 💦 @wing328 thanks for the reference! I learned from that. @ybelenko As the errors listed in #354 (comment) is due to Slim Framework (or its components), the generated API server seems work fine for me. Do you have any issues about the API server? |
…d by this upgrade.
… with all dependencies removed to keep repo more clean.
@wing328 @ackintosh |
👍 ✨ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the depencencies. LGTM!
chore(deps): update dependency @types/node to v14
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
and./bin/security/{LANG}-petstore.sh
if 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.1.x
,4.0.x
. Default:master
.Description of the PR
postProcessOperations
method overriden. Special value / properly escaped to avoid php syntax errors.LOGGER
like in Code clean-up: Add own private static final LOGGER in each class #26toModelName
copied from PHPClient codegen which is much more enhanced at that point.postProcessSupportingFileData
method overriden. Sort action fixes static route shadowing Bump junit from 4.13 to 4.13.1 in /samples/client/petstore/java/retrofit2-play26 #7698, but it sorts operations only within tag. It means that if user place pathappointmens/{id}
in first api group(tag Appointmets for example) andappointments/me
path in last group(tag MyResources), this sort function will produce broken stubs anyway.I've checked all endpoints with
petstore-with-fake-endpoints-models-for-testing.yaml
from Swagger UI and most of responses are correct with only few exceptions. I will add failed endpoints description in few days.cc @jfastnacht