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

Implement class serializers and interface to customize how objects are serialized #809

Merged
merged 24 commits into from
May 18, 2019

Conversation

stayallive
Copy link
Collaborator

@stayallive stayallive commented Apr 24, 2019

This allows the user (or framework library) to implement their own serialized values for any object instead of the Object SomeObject string that Sentry now shows.

Usage with interface (for object of your own):

use Sentry\Serializer\Serializable;

class ExampleObject implements Serializable
{
	private $state = false;

    public function toSentry(): array
    {
        return [
            'internal_state' => $this->state,
        ];
    }
}

Usage with serializer "helper" for objects you don't control, register via an option class_serializers.

use Sentry\ClientBuilder;
use Sentry\Options;

$serializers = [
    ExampleObject::class => static function (ExampleObject $object): array {
        return [
            'internal_state' => $object->getState(),
        ];
    },
];

$client = new ClientBuilder(new Options([
    'class_serializers' => $serializers,
]));

If a custom serializer or toSentry() generates an exception the value is discarded and the serializers serializes it as normal (resulting in a Object ExampleClass string). This is to prevent any issues caused by bad serializers or problems that occur there that might mask the actual exception that we are trying to send/serialize.

The class_serializers are resolved using instanceof to allow the user to add a custom serializer for a parent class or interface.

Currently it creates an array with the data nested to make it clear which class it actually is, this might be a concern because of the max depth but probably is not.

[
    // get_class($object) but without the 'Object' prefix
    'class' => 'ExampleObject',

    // Whatever the __toSentry() or custom serializer returned:
    'data' => [
        'internal_state' => false,
    ],
]

/ref #443 #316

@stayallive stayallive requested review from HazAT, ste93cry and Jean85 April 24, 2019 12:07
@Jean85 Jean85 added this to the 2.1 milestone Apr 26, 2019
Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Neat idea!

I would just object on two small naming things...

src/Serializer/Serializable.php Outdated Show resolved Hide resolved
src/Options.php Outdated Show resolved Hide resolved
@stayallive stayallive requested a review from Jean85 April 27, 2019 11:32
Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Perfect, LGTM now! 👍

@stayallive stayallive changed the title Implement type serializers option and interface to serialize objects Implement class serializers and interface to customize how objects are serialized May 3, 2019
@stayallive stayallive force-pushed the feature/serializable-objects branch from 322e77d to ea91bea Compare May 3, 2019 08:07
tests/Serializer/SerializerTest.php Outdated Show resolved Hide resolved
tests/Serializer/SerializerTest.php Outdated Show resolved Hide resolved
tests/Serializer/SerializerTest.php Outdated Show resolved Hide resolved
src/Options.php Outdated Show resolved Hide resolved
src/Options.php Outdated Show resolved Hide resolved
src/Serializer/AbstractSerializer.php Outdated Show resolved Hide resolved
@ste93cry
Copy link
Collaborator

ste93cry commented May 3, 2019

I see a few drawbacks in this implementation:

  • if the user wants to serialize something based not only on the type of the object but also on some other conditions he can't since resolving the serializer is based on the FQCN
  • if the user implements its own serializer and he's using the new option suddenly it won't work anymore without him having to reimplement all the logic. The same problem was unfortunately also caused forcibly by the introdction of the http_proxy option which only works with a certain transport and means that we are leaking parts of some features in the options where they should not stay

Said this I obviously see the value of such feature, but I'm not convinced that this is the way to go

@stayallive
Copy link
Collaborator Author

if the user wants to serialize something based not only on the type of the object but also on some other conditions he can't since resolving the serializer is based on the FQCN

The user get's the object passed so we could allow a null return value to skip the serializer and default back to the regular serialization to fix this?

if the user implements its own serializer and he's using the new option suddenly it won't work anymore without him having to reimplement all the logic. The same problem was unfortunately also caused forcibly by the introdction of the http_proxy option which only works with a certain transport and means that we are leaking parts of some features in the options where they should not stay

Their own serializer will work just fine... it won't magically replace their serializer. If they want to switch they can, if they don't they don't...

Said this I obviously see the value of such feature, but I'm not convinced that this is the way to go

Please convince me about a better way forward than 😉

@stayallive stayallive force-pushed the feature/serializable-objects branch from ea91bea to ce4461a Compare May 4, 2019 09:01
@stayallive
Copy link
Collaborator Author

I did make a change which allows the user to register a class_serializer to possibly overwrite the value of a SerializableInterface object.

Also allowing to return null from a serializer allows you to write multiple serializer for more specific classes if you wanted to or overwrite the SerializableInterface return for a certain object with a class_serializer.

The only "issue" here is sorting, because the order you register serializers in the class_serializer matters, if you first have Exception and next have a RuntimeException serializer (as an example) and the first serializer always returns something the RuntimeException serializer will never be called, switching them around would fix that. However as long as it's documented I don't think it's an issue worth fixing if it were fixable at all without serious performance penalties.

@stayallive stayallive requested a review from ste93cry May 4, 2019 09:09
@ste93cry
Copy link
Collaborator

ste93cry commented May 13, 2019

Thinking more about this feature, if someone wants to customize how objects are serialized or represented then the best way is to write their own serializer, which is why the SerializerInterface interface exists in the first place: to allow users to replace the implementation with something else and have full control over it. This option may be useful indeed because, but imho it's responsibility of the developer to make such customizations

CHANGELOG.md Outdated Show resolved Hide resolved
src/Options.php Outdated Show resolved Hide resolved
src/Options.php Outdated Show resolved Hide resolved
src/Options.php Outdated Show resolved Hide resolved
src/Options.php Outdated Show resolved Hide resolved
src/Serializer/SerializableInterface.php Show resolved Hide resolved
tests/Serializer/SerializerTest.php Outdated Show resolved Hide resolved
tests/Serializer/SerializerTest.php Outdated Show resolved Hide resolved
tests/Serializer/SerializerTest.php Outdated Show resolved Hide resolved
tests/Serializer/SerializerTest.php Outdated Show resolved Hide resolved
@stayallive stayallive requested a review from ste93cry May 15, 2019 08:40
@stayallive
Copy link
Collaborator Author

@ste93cry I think I got all your suggestions in, good ones!

@Jean85
Copy link
Collaborator

Jean85 commented May 15, 2019

Build failing due to CS.

Copy link
Collaborator

@ste93cry ste93cry left a comment

Choose a reason for hiding this comment

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

Still a few things left to fix, but overall it seems good

src/Options.php Outdated Show resolved Hide resolved
src/Options.php Outdated Show resolved Hide resolved
src/Serializer/AbstractSerializer.php Outdated Show resolved Hide resolved
src/Serializer/SerializableInterface.php Outdated Show resolved Hide resolved
tests/Serializer/SerializerTest.php Show resolved Hide resolved
tests/Serializer/SerializerTest.php Outdated Show resolved Hide resolved
tests/Serializer/SerializerTest.php Outdated Show resolved Hide resolved
tests/Serializer/SerializerTest.php Outdated Show resolved Hide resolved
tests/Serializer/SerializerTest.php Outdated Show resolved Hide resolved
@stayallive stayallive requested a review from ste93cry May 16, 2019 22:32
@stayallive stayallive force-pushed the feature/serializable-objects branch from 2aa572f to d4a0629 Compare May 16, 2019 22:35
This reverts commit 60e0417.
@ste93cry ste93cry merged commit 92c773f into master May 18, 2019
@ste93cry ste93cry deleted the feature/serializable-objects branch May 18, 2019 12:09
@mfn
Copy link
Contributor

mfn commented Jul 12, 2019

I find it confusing that you have to return an array from the custom class serializer.

My first attempt was:

    public static function serializeCarbon(CarbonInterface $carbon): string
    {
        return get_class($carbon) . ': ' . $carbon->toIso8601String();
    }

But I never received this value because of the following in \Sentry\Serializer\AbstractSerializer::serializeRecursively:

foreach ($classSerializers as $classSerializer) {
                    try {
                        $serializedObjectData = $classSerializer($value);

                        if (\is_array($serializedObjectData)) {
                            return [
                            'class' => \get_class($value),
                            'data' => $this->serializeRecursively($serializedObjectData, $_depth + 1),
                        ];
                        }
                    } catch (\Throwable $e) {
                        // Ignore any exceptions generated by a class serializer
                    }
                }
…

Note the if (\is_array($serializedObjectData)) {.

It you don't return an array, it's silently discard.

Is that really the intention 🤔

Simply doing this works:

    public static function serializeCarbon(CarbonInterface $carbon): array
    {
        return [$carbon->toIso8601String()];
    }

And turns out this way on Sentry:
image

It's the same but slightly differently.

I just… I found a very unexpected restriction 🤷‍♀️

Now I understand I can switch the whole serialize but… IMHO it would make sense to me to support simply returning strings here?

@stayallive
Copy link
Collaborator Author

stayallive commented Jul 12, 2019

It was a choice made. In hindsight might have been better to allow for strings to. Design was for serializing objects (key-value) in mind mostly.

I think we can broaden it to allow for strings or other scalar types without BC but not 100% sure. It does have to be a type supported by json_encode so some form of check should stay probably so were not trying to encode objects as json anyhow.

@Jean85 Jean85 mentioned this pull request Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants