Fix/job request deiscriminator

[woksin]

Added

• Created timestamp to JobState

Fixed

• Jobs RequestType discriminator seems to not work every #1686 - JobState bson serializer would occasionally fail when serializing Request object

Removed

• Request from JobStepState
• Name from JobState - Type is now effectively what Name was before

[woksin]

I'm a bit stuck here on how to actually do the Bson serializer part... @einari

[github-actions]

Docker Image for this PR:
https://ghcr.io/cratis/chronicle:10.9.9-pr1693.4de85fa

docker run ghcr.io/cratis/chronicle:10.9.9-pr1693.4de85fa

[einari]

I've done something that might be useful for this. In fact, this is something that was accidentally removed from ApplicationModel and should be put back.

Fundamentals has the concept of derived types, which also stores a discriminator. This is done using a Guid, but our case would be the String representation.

It leverages the Convention system in the MongoDB driver:

https://github.com/einari/MongoDB/blob/main/Source/DerivedTypeDiscriminatorConvention.cs

This is the serializer:

https://github.com/einari/MongoDB/blob/main/Source/DerivedTypeSerializer.cs

This is the provider that provides the serializer:

https://github.com/einari/MongoDB/blob/main/Source/DerivedTypeSerializerProvider.cs

[woksin]

The issue here is that we need to know the JobType when serializng the IJobRequest so we need a serializer for JobState as well? That's the part i don't understand, the problem is serializing the IJobRequest, we want to know it's concrete clr type consistently and we only know that through the JobType. As far as I understand that was the idea

[einari]

The discriminator approach would do just that. The only downside is that the JobType would then be part of the Request BSON object.

An option here would be to stick the JobType on the IJobRequest interface. Then the discriminator convention approach would be fine and we wouldn't need a custom serializer for JobState.

I think this option is fine. If we want to expose JobType on the JobState level as well, we would need to keep the two in sync. But I don't think we would need it on the JobState level, as the Request is not optional (if I remember correctly)

[woksin]

I understand what you're saying, however I think it's strange for the JobState to not have the JobType and the IRequest to have it instead. It's also a bit weird from a instantiation point of view for the IRequest when we make it we have to supply the JobType in the constructor, or use the IJobTypes in the IRequest implementations instead or something like that. It's just a bit strange

[woksin]

Would be nice if we could getting a reference to the parent bson document when we serialized the IJobRequest 😓

[woksin]

@einari BsonReaderBookmark did the trick for me :)

I have one question though in relation to removing Request from JobStepState,
We have this class in Setup project which I have no idea what does:

/// <summary>
/// Represents the <see cref="JsonConverter{T}"/> that can convert <see cref="JobStepState"/>.
/// </summary>
public class JobStepStateJsonConverter : TypeWithObjectPropertiesJsonConverter<JobStepState>
{
    /// <inheritdoc/>
    protected override IEnumerable<string> ObjectProperties => [nameof(JobStepState.Request)];
}

[einari]

Great.

That Json converter can probably be deleted. I believe it was there for the frontend APIs. But the APIs are now using the gRPC contracts and if we want to expose the request information in the APIs, we'll figure something out. Like we do :)

[woksin]

And now we also have the same issue for Orleans serialization, cannot serialize an interface 😢

Edit:
To be more specific it is the JsonSerializer used internally by orleans when desserializing a message that fails with this

[woksin]

Do we have to do kind of the same thing for the json serializer?

[einari]

Ahh.. Damn..
I guess the shortest path right now is to do a JsonConverter.

Ideally we should get rid of Json serialization of our internal types and create Orleans Codecs instead. But I have a sneaky suspicion that is a bigger fish to fry right now.

[woksin]

This is also very annoying, this is just the same problem as with the MongoDB serializer, where we cannot create a converter just for IJobRequest because we don't have JobType and no way to reference parent object. And it's even more anoying with JsonConverter for JobType because there I don't have access to the service provider so I cannot use IJobTypes either....

This just keeps spiraling 😭

[einari]

Well, depends.
What we've done with for instance the DerivedTypes and a few other things in Fundamentals is that we have a singleton instance we can get access to. This could be an internal field. And we would still access it normally for those things that can have it injected via the interface.

E.g.:

https://github.com/Cratis/Fundamentals/blob/169f8c52cfc50a070f048b8f7f53c6a2c82f85ce/Source/DotNET/Fundamentals/Serialization/DerivedTypes.cs#L24

In globals we use it:

https://github.com/Cratis/Fundamentals/blob/169f8c52cfc50a070f048b8f7f53c6a2c82f85ce/Source/DotNET/Fundamentals/Json/Globals.cs#L26

Furthermore, we could have the JsonConverter have 2 constructors, one that takes the IJobTypes as a dependency (making it testable with mocks) and a default one that forwards to this passing the JobTypes.Instance.

public class JobRequestConverter
{
    public JobRequestConverter() : this(JobTypes.Instance);

    public JobRequestConverter(IJobTypes jobTypes)
    {
    }
}

[woksin]

If this is the way we have to go then I guess we don't have much better alternatives 😄

[einari]

There are always alternatives :)
Like the one we have done in ApplicationModel:

https://github.com/Cratis/ApplicationModel/blob/main/Source/DotNET/Applications/Internals.cs

Its basically exposing the ServiceProvider internally.

This could then be set like we do in AppModel:

https://github.com/Cratis/ApplicationModel/blob/3f4d341941303c11219aa0d3014197cb83c37886/Source/DotNET/Applications/ApplicationBuilderExtensions.cs#L20

Either way, its not optimal and perfect. They're all trade-offs. Pragmatically speaking, its fine.

[einari]

But for this use case, its probably better with my previous suggestion, as the ServiceProvider is probably not ready at the time of setting up the serializer options.

[woksin]

fixes #1686