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

Java code-first SDK: support for Akka's Jackson migration #1332

Closed
octonato opened this issue Nov 17, 2022 · 10 comments · Fixed by #1760
Closed

Java code-first SDK: support for Akka's Jackson migration #1332

octonato opened this issue Nov 17, 2022 · 10 comments · Fixed by #1760
Assignees
Labels
java-sdk kalix-runtime Runtime and SDKs sub-team

Comments

@octonato
Copy link
Member

No description provided.

@octonato
Copy link
Member Author

In addition to Akka's Jackson migration, we could also imagine some tooling to map old typeUrl to need. Something that will free the users to deal with @TypeName upfront (see #1275).

@octonato octonato transferred this issue from another repository Dec 8, 2022
@octonato octonato transferred this issue from another repository Dec 8, 2022
@octonato octonato added the kalix-runtime Runtime and SDKs sub-team label Jan 18, 2023
@efgpinto efgpinto self-assigned this Jan 18, 2023
@efgpinto
Copy link
Member

I was doing some experiments mainly for my own understanding of what was possible already or not, so here are some notes here:

  • add/removing fields from state types seems mostly fine: the fields will be null when non-existing and logic to handle that needs to live on the UF.
  • class renaming is fine as long as structure matches new type (or type info is used for ES events)

Akka Jackson migration will be needed to:

  • support renaming fields
  • support transforming fields and changing type (works for some primitive type conversions but fails for composite ones as expected)

@octonato
Copy link
Member Author

Json behaves similar to proto with respect to unknown fields. The main issue is renaming indeed.

Renaming is no issue for gRPC, only ordering matters. Changing type is a major problem for gRPC as well. And I don't think we should even try to solve this kind of problems.

Good that Akka Jackson Migration have already some support for it, even if limited that's already something. We are fine as long as we document the limitations.

Speaking about documenting migration, I don't think we have anything about protobuf and we simply expect that users will know how to evolve their protobuf. That's probably too optimistic.

I hear often that schema evolution is not an issue for protobuf. That's true only if you know what you are doing. The reality, is that very often people change their format without much care.

@johanandren
Copy link
Contributor

Side note: Re the protobuf schema evolution, we don't really link to it from our docs (we should), but there is information in the protobuf documentation: https://protobuf.dev/programming-guides/proto3/#updating

@efgpinto efgpinto removed their assignment Jan 31, 2023
@ennru ennru changed the title Spring SDK: support for Akka's Jackson migration Java code-first SDK: support for Akka's Jackson migration May 2, 2023
@octonato octonato added DX Developer Experience related issue and removed DX Developer Experience related issue labels Jul 24, 2023
@aludwiko aludwiko self-assigned this Aug 1, 2023
@aludwiko
Copy link
Contributor

aludwiko commented Aug 7, 2023

Ok, I'm experimenting with a similar approach to Akka Jackson Migration. The biggest challenge is how to save the version number. My idea is to use the typeUrl for that, e.g. json.kalix.io/com.example.SimpleClass#1 after hash I will add the version number. So far it looks promising, but if you see any blockers, please let me know:)

@aludwiko
Copy link
Contributor

aludwiko commented Aug 7, 2023

@TypeName annotation, currently used only for events, is there a blocker to use for the entity state? Or even for an endpoint JSON body?

@aludwiko aludwiko linked a pull request Aug 8, 2023 that will close this issue
@aludwiko
Copy link
Contributor

aludwiko commented Aug 9, 2023

While working on this I've found a bug: https://github.com/lightbend/kalix-proxy/issues/2093 Even after fixing it, users might fall into a very problematic scenario. Let's imagine that I have a Created event which is then published, could be S2S, Kafka, doesn't matter.

@Subscribe.EventSourcedEntity(
  value = CustomerEntity.class,
  ignoreUnknown = true)
@Publish.Stream(id = "customer_events") 
public class CustomerEventsService extends Action {

  public Effect<CustomerPublicEvent.Created> onEvent(Created created) {
    System.out.println("publshing" + created);
    return effects().reply(
      new CustomerPublicEvent.Created(created.email(), created.name()));
  }

I'm deploying a new version of the app, with renamed event Created2.

@Subscribe.EventSourcedEntity(
  value = CustomerEntity.class,
  ignoreUnknown = true)
@Publish.Stream(id = "customer_events") 
public class CustomerEventsService extends Action {

  public Effect<CustomerPublicEvent.Created> onEvent(Created2 created) {
    System.out.println("publshing" + created);
    return effects().reply(
      new CustomerPublicEvent.Created(created.email(), created.name()));
  }

In case of lagging, there could be a situation where the subscription will receive (after the restart) events:
Created, Created, Created2, Created2, ...

Because ignoreUnknown = true the old events will be silently ignored, which in fact is not what we are excepting.

To know about the problem we would need to recover some old entity with the old event name. Unfortunately applying proper migration will solve only the recovery aspect, the subscription will continue the work. Except for not using (or carefully using) the ignore option I don't think there is a good solution for that.

We should strongly recommend the TypeName annotation, which is also a solution to this problem.

@aludwiko
Copy link
Contributor

One interesting thing about Views + Migration is that we can now support the schema evolution of VE state model. For instance, we could have:

public class CustomerEntity extends ValueEntity<Customer> 

after some time this could be changed to:

@Migration(CustomerUpdatedMigration.class)
public record CustomerUpdated()
...
public class CustomerEntity extends ValueEntity<CustomerUpdated> 

A view can be created with a dummy transformation, that just returns the deserialized state, and the CustomerUpdatedMigration will do the job on the fly.

public class CustomerByEmailView extends View<CustomerUpdated> {
  @Subscribe.ValueEntity(CustomerEntity.class)// <3>
  public UpdateEffect<CustomerUpdated> transform(CustomerUpdated customer) {
    return effects().updateState(customer);
  }
}

@aludwiko
Copy link
Contributor

Ok, so @TypeName would work out of the box for all cases. For entities state it actually does matter. The type is always from the Java generics. As long the JSON schema is the same, the decoding will work. With workflows, the situation is a bit different, because we are persisting also step inputs and outputs, so we could emphasize thinking about ser/der more.

@aludwiko
Copy link
Contributor

aludwiko commented Aug 18, 2023

Another interesting observation. Let's say we have a Wallet Entity:

public class WalletEntity extends ValueEntity<WalletEntity.Wallet> {

   @JsonTypeInfo(use = JsonTypeInfo.Id.NAME)
  @JsonSubTypes({
      @JsonSubTypes.Type(value = WithdrawResult.WithdrawSucceed.class),
      @JsonSubTypes.Type(value = WithdrawResult.WithdrawSucceed.class)})
  public sealed interface WithdrawResult {
    record WithdrawFailed(String errorMsg) implements WithdrawResult {
    }

    record WithdrawSucceed() implements WithdrawResult {
    }
  }


  @PatchMapping("/withdraw/{amount}")
  public Effect<WithdrawResult> withdraw(@PathVariable int amount) {
    Wallet updatedWallet = currentState().withdraw(amount);
    if (updatedWallet.balance < 0) {
      return effects().reply(new WithdrawFailed("Insufficient balance"));
    } else {
      return effects().updateState(updatedWallet).thenReply(new WithdrawSucceed());
    }
  }

and we are calling the withdraw endpoint from the same service. In that case, we don't need any @JsonSubTypes annotations, because the Any.urlType gives us all the information required to deserialize such a payload. Unless we transform the deferred call to a CompletionStage:

            componentClient.forValueEntity(cmd.from)
            .call(WalletEntity::withdraw)
            .params(cmd.amount)
            .execute()

If so, the deserialization is done via Spring Web Client internals without (our) JsonSupportutility. Here @JsonSubTypes is required to produce a proper payload and to deserialize it later.

If we do the same, but the call will be to an external Kalix service. Not only should we recommend using @TypeName annotation, but also @JsonSubTypes annotation with hardcoded type name:

@JsonSubTypes.Type(value = WithdrawResult.WithdrawSucceed.class, name = "withdraw-succeed"),

to avoid sending FQCN. We are talking about mixed usage: rest calls + deferred calls.

JSON migration, unless we tweak the Web Client, applies only to deferred calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java-sdk kalix-runtime Runtime and SDKs sub-team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants