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

[BUG] [PHP] [Symfony] Several issues for parameters of type date-time #14930

Closed
5 of 6 tasks
gturri opened this issue Mar 11, 2023 · 1 comment
Closed
5 of 6 tasks

[BUG] [PHP] [Symfony] Several issues for parameters of type date-time #14930

gturri opened this issue Mar 11, 2023 · 1 comment

Comments

@gturri
Copy link
Contributor

gturri commented Mar 11, 2023

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

When I declare a parameter of with type: string and format: date-time and generate a symfony bundle with it, then I observe that queries on that endpoint get a 400 response with message This value should be of type string.

openapi-generator version

I've observed this behavior with openapi-generator 6.3.0, as well as on a version built from source with today's master branch (140d941)

OpenAPI declaration file content or url
openapi: 3.0.1
info:
  title: Test
  version: 0.0.1
paths:
  /my-route:
    get:
      summary: Get stuff
      parameters:
        - name: myparam
          in: query
          required: false
          schema:
            type: string
            format: date-time
      responses:
        "200":
          description: OK
Generation Details
java -jar openapi.jar generate -i openapi.yaml -g php-symfony -o out
Steps to reproduce

Plug the symfony bundle generated into a symfony app (following the steps in the auto-generated README). Then launch the symfony server and perform a query against this route, for instance:

  curl -i http://localhost:8000/my-route # this should be ok because the parameter is marked as non required
  # or also
  curl -i 'http://localhost:8000/my-route?myparam=2017-07-21T17:32:28Z'

Expected behavior: I receive a 200 response.
Actual behavior: I receive a 400 response with content This value should be of type string`

Related issues/PRs

I've looked at https://github.com/OpenAPITools/openapi-generator/issues?q=is%3Aissue+symfony+date-time but those issues are not related to mine.

Suggest a fix

I think thos 3 changes may be needed to fix that issue:

  1. Currently the generated Controller/DefaultController.php looks like that:
        // Read out all input parameter values into variables
        $myparam = $request->query->get('myparam');

        // Use the default value if no value was provided

        // Deserialize the input values that needs it
        try {
            $myparam = $this->deserialize($myparam, '\DateTime', 'string');
        } catch (SerializerRuntimeException $exception) {
            return $this->createBadRequestResponse($exception->getMessage());
        }

        // Validate the input values
        $asserts = [];
        $asserts[] = new Assert\DateTime();
        $response = $this->validate($myparam, $asserts);
        if ($response instanceof Response) {
            return $response;
        }

Here we see that the validation occurs after the deserialization. That explains the issue observed since the validation expects $myparam to be a string but it was already deserialized to DateTime. So I think the validation part should go before the deserialization, so that $myparam would still be a string at the moment where it is checked.

  1. In the snippet above we see that the generated code relies on Symfony's Symfony\Component\Validator\DateTime to perform the assertion, however that class checks against the format Y-m-d H:i:s though open api specification considers that date-time should conform to RFC 3339 which would be for instance 2017-07-21T17:32:28Z. So I think this check should be adapted as well (not sure by what it should be replaced though)

  2. The generated file service/JmsSerializer.php has a method deserializeString which returns new DateTime($data) for parameter of type date-time. But since new DateTime(null) represents "now" it means that if the parameter is not provided by the query (which should be ok since I marked it has non required) then the deserialization would return "now" instead of null. (perhaps this handling of null $data should also be applied to other format of strings? I don't know)

gturri added a commit to gturri/openapi-generator that referenced this issue Mar 12, 2023
This fixes parts of OpenAPITools#14930.

Without this patch a parameter declared as date-time
is validated against Symfony's "DateTime" constraint,
which always fails. Because this constraint expects
a string with format "Y-m-d H:i:s".
It fails because the generated code performs the check
after the deserialization, so the variable checked is not
a string anymore.

Besides this, even if we performed that validation on the
string, that would not work well because OpenApi
specification expects date-time to conform to RFC 3339
and that "Y-m-d H:i:s" would reject RFC 3339 compliant dates.

With this change we ensure that the string provided by the
web user could be parsed by PHP to DateTime, which solves both issues.

(Note however that it means that it now accepts more formats than just
RFC 3339 compliant ones for those parameters (it would accept all formats
accepted by PHP DateTime). That being said it's compliant with the guideline
""be conservative in what you send, be liberal in what you accept")
@gturri
Copy link
Contributor Author

gturri commented Mar 12, 2023

I gave those issues some more though, and I think it can all be fixed with minimal changes with PR #14933.
At least I've tested it locally and it now works fine. And since it is done with so little changes I'm rather confident that it should not have unexpected side effects.

gturri added a commit to gturri/openapi-generator that referenced this issue Mar 12, 2023
This fixes one of the issue described on OpenAPITools#14930, namely that
the deserializeString method of the generated class JsmSerializer returns null
for other types of string, but not for date-time. Instead it returns a DateTime
which represents "now" (because that what `new DateTime(null)` does).

Consequently when an API declares a date-time parameter as non-required and
when that endpoint is called without that parameters, then the user code
would end up having "now" instead of "null" for this parameter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant