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

Make Joda Money embeddable #340

Merged
merged 1 commit into from Nov 7, 2019

Conversation

@jianglai
Copy link
Member

jianglai commented Nov 1, 2019

This PR adds XML mappings to make Joda Money an embeddable field. When an entity contains a Money field is persisted, it will be stored in two fields (amount, currency). If the column names collide with other columns, they can be re-named on a case-by-base basis using @AttributeOverrides.

See JodaMoneyConverterTest.java for an example. If not all columns of Money need to be overridden, the @AttirbuteOverrides annotation can include just one @AttributeOverride.

Money can also be part of a collection (values in a map for instance) and to override the the columns the name filed in the @AttributeOverride annotation needs to be prefixed with value.. See examples in JodaMoneyConverterTest.java.

Note that when Money is a field persisted in an entity, a @PostLoad callback is necessary to set the amount to the correct scale according to the currency after the entity is loaded from the database. See the test file for examples.

Also updates Joda Money version to 1.0.1 which includes an no-op default constructor for Money and BigMoney so that Hibernate can instantiate them reflectively (see JodaOrg/joda-money#23).


This change is Reviewable

@jianglai jianglai requested review from weiminyu and mindhog Nov 1, 2019
@googlebot googlebot added the cla: yes label Nov 1, 2019
@jianglai jianglai force-pushed the jianglai:converter branch 5 times, most recently from 314b1e4 to 6cce31d Nov 1, 2019
Copy link
Member

mindhog left a comment

Reviewed 62 of 62 files at r1, 59 of 59 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jianglai and @weiminyu)


core/src/test/java/google/registry/persistence/JodaMoneyConverterTest.java, line 43 at r1 (raw file):

 * fields: a {@link BigDecimal} {@code amount} and a {@link CurrencyUnit} {@code currency}. When we
 * store an entity with a {@link Money} field, we would like to store it in two columns, for the
 * amount and the currency separately, so that it is easily querable. This requires that we make

*queryable


core/src/test/java/google/registry/persistence/JodaMoneyConverterTest.java, line 105 at r1 (raw file):

            200, BigDecimal.valueOf(100).setScale(CurrencyUnit.USD.getDecimalPlaces()), "USD")
        .inOrder();
    TestEntityWithOtherAmount persisted =

I presume this is to verify no conflict with columns of the same name. Can you add a comment here to indicate this?

@jianglai jianglai requested a review from mindhog Nov 3, 2019
Copy link
Member Author

jianglai left a comment

Reviewable status: 62 of 63 files reviewed, 2 unresolved discussions (waiting on @mindhog and @weiminyu)


core/src/test/java/google/registry/persistence/JodaMoneyConverterTest.java, line 43 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

*queryable

Done.


core/src/test/java/google/registry/persistence/JodaMoneyConverterTest.java, line 105 at r1 (raw file):

Previously, mindhog (Michael Muller) wrote…

I presume this is to verify no conflict with columns of the same name. Can you add a comment here to indicate this?

Done.

Copy link
Member

CydeWeys left a comment

Given that the majority of files changed are caused solely by bumping the version of Joda Money, and also considering that there might be consequences from doing so, I think it makes sense to split that out into a separate PR (should probably go in first). That way it'll be easier to evaluate the consequences of that change in isolation.

Reviewable status: 62 of 63 files reviewed, 2 unresolved discussions (waiting on @mindhog and @weiminyu)

Copy link
Member

CydeWeys left a comment

Reviewable status: 62 of 63 files reviewed, 4 unresolved discussions (waiting on @jianglai, @mindhog, and @weiminyu)


core/src/main/resources/META-INF/orm.xml, line 9 at r4 (raw file):

  <embeddable class="org.joda.money.Money">
    <attributes>
      <embedded name="money"/>

Does this mean it only works on columns named "money"?


core/src/main/resources/META-INF/orm.xml, line 14 at r4 (raw file):

  <embeddable class="org.joda.money.BigMoney">
    <attributes>
      <basic name="amount"/>

These aren't configurable? It's always mapped to "amount" and "currency"?

Does this approach work for the EAP schedule, which is a Map of DateTimes to Moneys?

What happens if you have multiple Moneys in a given object? The Registry entity, for example, has several different Moneys, corresponding to different costs of creating, renewing etc.

Can the amount and currency be grouped together in some way? It's possible in PostgreSQL to do things in such a way that queries would look like "SELECT money.amount, money.currency ...". That way if there's multiple different fields of type Money, they're grouped together correctly.

@jianglai jianglai requested a review from CydeWeys Nov 3, 2019
Copy link
Member Author

jianglai left a comment

You have a good point. But this PR doesn't add any production Java code other than the change to bump Joda Money version. It only adds an xml file along with tests, which won't change production in anyway because we haven't persisted any entity containing Joda Money yet. So there really isn't anything else to isolate.

Reviewable status: 62 of 63 files reviewed, 4 unresolved discussions (waiting on @CydeWeys, @mindhog, and @weiminyu)


core/src/main/resources/META-INF/orm.xml, line 9 at r4 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Does this mean it only works on columns named "money"?

See below.


core/src/main/resources/META-INF/orm.xml, line 14 at r4 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

These aren't configurable? It's always mapped to "amount" and "currency"?

Does this approach work for the EAP schedule, which is a Map of DateTimes to Moneys?

What happens if you have multiple Moneys in a given object? The Registry entity, for example, has several different Moneys, corresponding to different costs of creating, renewing etc.

Can the amount and currency be grouped together in some way? It's possible in PostgreSQL to do things in such a way that queries would look like "SELECT money.amount, money.currency ...". That way if there's multiple different fields of type Money, they're grouped together correctly.

These columns are configurable. But obviously we can't do that in the xml because then all Money entities will use that, defeating the purpose of configuring them in the first place.

What you need to do is to use @AttibuteOverrides to annotate the Money field in your entity and customize the column names. If you look at JodaMoneyConverterTest you can see an example test to verify that such configuration works.

I don't think you can group them in a SQL native way. If it were BigQuery, you can query nested entities like that, but that's a NoSQL database in disguise. The best we can do is probably to give them sensible names to make it clear that a certain amount is to be associated with a certain currency.

I haven't thought about if it works with maps. I think it has more to do with how Hibernate persists maps. What this CL does is to make it so that whenever you have a Money field it is persisted in two columns in SQL, which is parallel to how maps with certain types as keys or values are persisted. Do we have any example of maps that are already persisted in SQL?

@mindhog
mindhog approved these changes Nov 4, 2019
Copy link
Member

mindhog left a comment

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @weiminyu)

Copy link
Collaborator

weiminyu left a comment

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys)


core/src/main/resources/META-INF/orm.xml, line 14 at r4 (raw file):

Previously, jianglai (Lai Jiang) wrote…

These columns are configurable. But obviously we can't do that in the xml because then all Money entities will use that, defeating the purpose of configuring them in the first place.

What you need to do is to use @AttibuteOverrides to annotate the Money field in your entity and customize the column names. If you look at JodaMoneyConverterTest you can see an example test to verify that such configuration works.

I don't think you can group them in a SQL native way. If it were BigQuery, you can query nested entities like that, but that's a NoSQL database in disguise. The best we can do is probably to give them sensible names to make it clear that a certain amount is to be associated with a certain currency.

I haven't thought about if it works with maps. I think it has more to do with how Hibernate persists maps. What this CL does is to make it so that whenever you have a Money field it is persisted in two columns in SQL, which is parallel to how maps with certain types as keys or values are persisted. Do we have any example of maps that are already persisted in SQL?

ClaimsList.labelsToKeys is a Map<String, String> field. We may want to verify that the same approach works with custom type too.

Copy link
Member

CydeWeys left a comment

The commit description needs more information on how to use this. It says to use @AttributeOverrides, but doesn't say how, nor provide an example.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @CydeWeys, @jianglai, and @weiminyu)


core/src/main/resources/META-INF/orm.xml, line 14 at r4 (raw file):

Previously, weiminyu (Weimin Yu) wrote…

ClaimsList.labelsToKeys is a Map<String, String> field. We may want to verify that the same approach works with custom type too.

So what is our strategy for solving EAP? (Or maybe even Maps in Cloud SQL generally?) I'm wondering if this will help at all with EAP.


core/src/test/java/google/registry/persistence/JodaMoneyConverterTest.java, line 137 at r4 (raw file):

    @AttributeOverrides(
        @AttributeOverride(name = "money.amount", column = @Column(name = "money_amount")))

It's not clear to me how this works re: the currency field. Should be elaborated in the commit description what exact columns an example @AttributeOverrides maps to.

@jianglai jianglai force-pushed the jianglai:converter branch from 110718f to 8fd8b72 Nov 4, 2019
Copy link
Member

CydeWeys left a comment

Reviewable status: 62 of 63 files reviewed, 3 unresolved discussions (waiting on @CydeWeys, @jianglai, @mindhog, and @weiminyu)


core/src/main/resources/META-INF/orm.xml, line 14 at r4 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

So what is our strategy for solving EAP? (Or maybe even Maps in Cloud SQL generally?) I'm wondering if this will help at all with EAP.

And if this doesn't work for more complex things, but something else might, maybe this is a dead end?

@mindhog
mindhog approved these changes Nov 5, 2019
Copy link
Member

mindhog left a comment

Reviewed 3 of 3 files at r5.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @CydeWeys, @jianglai, and @weiminyu)

@jianglai jianglai requested a review from CydeWeys Nov 5, 2019
Copy link
Member Author

jianglai left a comment

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @CydeWeys)


core/src/main/resources/META-INF/orm.xml, line 14 at r4 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

And if this doesn't work for more complex things, but something else might, maybe this is a dead end?

I spent the better half of yesterday experimenting with this and it turned out this works. There's a hidden bug in Hibernate that complicates things (see here and here) when you have nested embeddables inside a collection. The solution is to explicitly set the access mode for the nested embeddables to be FIELD, otherwise Hibernate will usePROPERTY access and fail to find the setters. See examples in the test.


core/src/test/java/google/registry/persistence/JodaMoneyConverterTest.java, line 137 at r4 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

It's not clear to me how this works re: the currency field. Should be elaborated in the commit description what exact columns an example @AttributeOverrides maps to.

Added overrides for both columns.

@jianglai

This comment has been minimized.

Copy link
Member Author

jianglai commented Nov 5, 2019

OK there is one more problem -- Money will set the scale of the BigDecimal in BigMoney depending on the currency. So JPY has scale 0 and USD has size 2. If we have mixed currencies stored in the same table (e. g. Registry) the column for amount can only have one scale. If we set it to scale 2 (the default for BigDecimal), the Money loaded from DB will all have scale 2 and breaks the contract for JPY. This would be solvable if we control the Money class. We can add a @PostLoad method to manually correct the size based on the currency.

We can either remember to add a @PostLoad to each entity that contains Money and correct the scale, or load a default @EntityListener for all entities, which reflectively iterate through all fields in any entity and fix the scale.

Both of these are not ideal.

@jianglai jianglai force-pushed the jianglai:converter branch from afd5211 to 49badc6 Nov 6, 2019
@jianglai

This comment has been minimized.

Copy link
Member Author

jianglai commented Nov 6, 2019

PTAL. I added an example on how to use the @PostLoad callback to correct the scale.

Copy link
Collaborator

weiminyu left a comment

Reviewed 2 of 62 files at r1, 57 of 59 files at r2, 4 of 4 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @CydeWeys)

@jianglai jianglai dismissed CydeWeys’s stale review Nov 7, 2019

Added tests to demo that embedded Joda Money works for complex data structure works.

@jianglai jianglai merged commit 64e7a59 into google:master Nov 7, 2019
4 of 7 checks passed
4 of 7 checks passed
code-review/reviewable 3 discussions left (CydeWeys)
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: Java No new or fixed alerts
Details
cla/google All necessary CLAs are signed
kokoro-foss Kokoro build finished
Details
kokoro-internal Kokoro build finished
Details
@jianglai jianglai deleted the jianglai:converter branch Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.