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

JsonDeserializer can't handle all RFC3339 timestamps #35

Closed
anniefu opened this issue Nov 2, 2021 · 6 comments · Fixed by #37
Closed

JsonDeserializer can't handle all RFC3339 timestamps #35

anniefu opened this issue Nov 2, 2021 · 6 comments · Fixed by #37
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@anniefu
Copy link

anniefu commented Nov 2, 2021

The TimeFormatter class causes error when deserializing CloudEvents. It does not accept all of the valid examples given in https://www.rfc-editor.org/rfc/rfc3339

The PHP RFC3339 implementation should probably be used instead.

Repro:

<?php
require 'vendor/autoload.php';

use GuzzleHttp\Psr7\ServerRequest;
use CloudEvents\Serializers\JsonDeserializer;

$request = new ServerRequest(
   'POST',
   '',
   [],
   '{
      "specversion": "1.0",
      "type": "google.firebase.database.ref.v1.written",
      "source": "//firebasedatabase.googleapis.com/projects/_/locations/europe-west1/instances/my-project-id",
      "subject": "refs/gcf-test/xyz",
      "id": "aaaaaa-1111-bbbb-2222-cccccccccccc",
      "time": "1985-04-12T23:20:50.52Z",
      "datacontenttype": "application/cloudevents+json",
      "data": {
         "data": {
            "grandchild": "other"
         },
         "delta": {
            "grandchild": "other changed"
         }
      }
   }'
);

$event = JsonDeserializer::create()->deserializeStructured($request->getBody());
echo $event;

Results in:

PHP Fatal error:  Uncaught ValueError: CloudEvents\Utilities\TimeFormatter::decode(): Argument #1 ($time) is not a valid RFC3339 timestamp in /usr/local/google/home/anniefu/code/testing/php/vendor/cloudevents/sdk-php/src/Utilities/TimeFormatter.php:37
@jlaswell
Copy link
Contributor

jlaswell commented Nov 9, 2021

You are correct that we should update support here. I'll backlog an item to make this change to support the time-fraction portion of the spec.

@jlaswell jlaswell self-assigned this Nov 9, 2021
@jlaswell jlaswell added bug Something isn't working good first issue Good for newcomers labels Nov 9, 2021
@jlaswell jlaswell pinned this issue Nov 9, 2021
@GrahamCampbell
Copy link
Contributor

I've dug into the spec a bit more, and it also says that the T and Z parsing should not be case sensitive, so we possibly have another issue there, too.

@anniefu
Copy link
Author

anniefu commented Dec 10, 2021

Perhaps just use PHP's native implementation for formatting timestamps? PHP RFC3339

@GrahamCampbell
Copy link
Contributor

Unfortunately PHP's native implementation is incorrect, and they couldn't fix it for BC reasons. :trollface:

@GrahamCampbell
Copy link
Contributor

I should be able to give us some kind of middleground here.

@GrahamCampbell
Copy link
Contributor

GrahamCampbell commented Dec 10, 2021

https://3v4l.org/mkQKd is closer, but still not right, since the php extended constant rejects things the spec allows. The spec says you can have as many numbers as you like after the period...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
3 participants