Improve Json Handler by removing the specific serialization of Key.class

[alallema]

Description:

Currently when serializing data via the encode method of the JSON handler the class Key class is treated separately because of the date format.
We should find a solution so that all the dates are well managed in the JSON handler

Condition to remove:

    if (o != null && o.getClass() == Key.class)

[ginaesps]

Hello! Is this issue still available?

[alallema]

Hi @ginaesps,
Yes, this issue is still available!

[AtaydeEnrique]

Hello I'd like to work in this issue. What it needs to be done here is implementing a separate serialization method for the Key class, is it correct?

[alallema]

Hi @AtaydeEnrique,
Not really for me, the solution would be to manage all the dates correctly via the JSON handler, not just a special case for the Key class.

[joseearias]

Hi! Seeing as it has been a couple of weeks without any updates I would like to try to find a solution for this issue, if it's ok.

[alallema]

Hi @joseearias,
It's more ok that would be great! Thanks

[joseearias]

Perfect. Thanks! I'll leave an update as soon as something comes up.

[jrhenderson1988]

Hi @alallema,

I've just had a little dig into this issue, as I noticed that there has been no progress on this it the last update.

There seems to be two JSON handlers in the project, one which uses GSON (the one in question for this issue) and another using Jackson. Switching the default to use Jackson avoids this piece of code altogether. Jackson is pretty flexible and the configuration already present in the project works pretty well (@JsonInclude(Include.ALWAYS) on the expiresAt field)

Do you know why there are two JSON handlers? Would it be feasible to switch the default implementation to use Jackson instead of GSON and potentially remove the GSON version? (that could be a breaking change and may require a major version change)

If the plan is to keep the existing GSON handler, then I think the code that's present right now does the job, although it is not ideal. It may be possible to modify it a little bit to use a custom serializer instead, but I suppose that mostly just moves the conditional code from the GSON handler class, into another place (the customer serializer), so it might not be a particularly large improvement.

I'd be happy to help contribute either approach, or if you have another idea for improvements. Whatever you think is best for the project.

[alallema]

Do you know why there are two JSON handlers?

Yes, I know why there are two, there were originally several JSON handlers to avoid the user having to import another JSON library than the one he used in his project. When I refactored the SDK, I left two so there would be a minimum of choice, but it doesn't seem to have been a good decision.

Would it be feasible to switch the default implementation to use Jackson instead of GSON and potentially remove the GSON version? (that could be a breaking change and may require a major version change)

I think one will be enough and Jackson is a good choice for me. However, the modification will bring a breaking change and a lot of changes, I think it's up to @brunoocasali or @curquiza to give their opinion on it.

[jrhenderson1988]

Thanks for getting back to me with that information @alallema

I'll wait for a response from @brunoocasali or @curquiza on how you'd like to proceed before starting any work on this.

[brunoocasali]

Hi @jrhenderson1988 @alallema

Both libraries are solid and could be easily chosen between one or the other. I don't want to introduce a new breaking change again in this library, which has a turbulent past 😅

Gson is the default implementation, but it is not included in the final bundle, and the implementation is just a layer to adapt the SDK to those JSON libraries as far as I'm concerned.

So, I don't believe we made a mistake here, just that specialization of the Key.class that, for me, shouldn't be there 😃

CC: @curquiza

[jrhenderson1988]

No problem @brunoocasali. I'd be happy to work on this while trying to avoid making breaking changes. I noticed that the code in place right now has a lot to do with null checking and it seems that it's important that the expiresAt value is always present.

Are you or perhaps @alallema able to explain what the full requirements are in this case? How should it work exactly? I just want to make sure that I understand the desired effect before I invest any significant effort.

Thanks.

[brunoocasali]

@alallema could help by providing details of the implementation!

But to me, keeping the current behavior without explicitly specifying the Key class in that portion of the code would be enough.

Why?

Since the method encode is part of that adapter layer, it shouldn't have any specific code there. And if we wanted to handle a different case, we should do that in another place. So basically, if we could have a way to move this code without breaking the current behavior, it would be nice.

[alallema]

Hi @jrhenderson1988,

Thank you so much for taking care of this.

I noticed that the code in place right now has a lot to do with null checking and it seems that it's important that the expiresAt value is always present.

Yes, actually the issue is that some values are needed to create a Key as actions, indexes, and expiresAt. In the case of expiresAt, this value may be null but it has to be set by the user anyway.

This is where it gets tricky, as it means that the class must ensure not to set all variables to null but only those set to null by the user because it's possible to set your key with an expiresAt at null.

In addition, we also use this variable when updating a key, which must pass through the same Key class.

I hope this answers your questions and above all that it was clear enough. Don't hesitate if it's not the case

[jrhenderson1988]

Hi @alallema @brunoocasali

I've submitted a PR that hopefully does what you're looking for. I've removed the key specific logic from the class and endeavoured to keep the existing functionality as it is now. I've added more tests (which I ran against the current code also) to verify that the functionality is the same, though it is possible that I may have missed something.

Thanks for your help