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

Treat value classes as such #610

Merged
merged 14 commits into from
Oct 14, 2022
Merged

Conversation

turb
Copy link
Contributor

@turb turb commented Sep 27, 2022

The new Neo4J support works well, but value classes generate inner properties in Neo4J, when we would like them to be just strongly typed values.

@turb
Copy link
Contributor Author

turb commented Oct 3, 2022

cc @RustedBones @clairemcginty

@RustedBones RustedBones self-assigned this Oct 5, 2022
@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

Merging #610 (69b0596) into main (a368f04) will decrease coverage by 1.22%.
The diff coverage is 88.52%.

@@            Coverage Diff             @@
##             main     #610      +/-   ##
==========================================
- Coverage   96.11%   94.89%   -1.23%     
==========================================
  Files          48       48              
  Lines        1649     1724      +75     
  Branches      152      177      +25     
==========================================
+ Hits         1585     1636      +51     
- Misses         64       88      +24     
Impacted Files Coverage Δ
...ed/src/main/scala/magnolify/shared/Converter.scala 42.85% <0.00%> (-11.69%) ⬇️
...src/main/scala/magnolify/parquet/ParquetType.scala 94.27% <76.92%> (-2.49%) ⬇️
...o4j/src/main/scala/magnolify/neo4j/ValueType.scala 83.33% <80.00%> (-1.42%) ⬇️
...c/main/scala/magnolify/bigtable/BigtableType.scala 98.03% <87.50%> (-1.28%) ⬇️
...c/main/scala/magnolify/bigquery/TableRowType.scala 94.93% <91.42%> (-2.17%) ⬇️
avro/src/main/scala/magnolify/avro/AvroType.scala 96.42% <92.10%> (-1.63%) ⬇️
...c/main/scala/magnolify/protobuf/ProtobufType.scala 96.33% <92.30%> (-2.67%) ⬇️
.../main/scala/magnolify/tensorflow/ExampleType.scala 93.68% <92.85%> (-1.72%) ⬇️
...rc/main/scala/magnolify/datastore/EntityType.scala 93.75% <93.75%> (-1.71%) ⬇️
...la/magnolify/guava/semiauto/FunnelDerivation.scala 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@RustedBones
Copy link
Contributor

This actually affects all modules.
I agree with @turb that AnyVals should not be visible in the destination model after conversion.

Comment on lines 90 to 93
val v: Value = new StringValue("Hello, world")
val a = new MapValue(Map("vc" -> v).asJava)
val c = vt.from(a)
assert(c == HasValueClass(ValueClass("Hello, world")))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this part is useless (it is already tested in the test while doing the round-trip)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, since the roundtrip will not test the absence of sub-property in the serialized version. Without the change, it will serialize has { "vs": { "str": "Hello, world" } } and deserialize to match the original, and the test will pass, even if the case class is an AnyVal.

Copy link
Contributor

@RustedBones RustedBones Oct 5, 2022

Choose a reason for hiding this comment

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

What I mean, is that in the line above is enough

assert(record.get("vc").asString() == "String")

We are checking that the value class does not show in the target model.
Since test checks the round trip, we know that the conversion back to scala will create the value class in the HasValueClass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yeah, the last assert may be seen as redundant with test + the previous assert. I would be in favor of testing this explicitely, however that's your call.

neo4j/src/test/scala/magnolify/neo4j/ValueTypeSuite.scala Outdated Show resolved Hide resolved
@RustedBones RustedBones changed the title Neo4J: treat value classes as such Treat value classes as such Oct 5, 2022
Co-authored-by: Michel Davit <michel@davit.fr>
@turb
Copy link
Contributor Author

turb commented Oct 5, 2022

This actually affects all modules. I agree with @turb that AnyVals should not be visible in the destination model after conversion.

Agreed, although for others it will be a breaking change. For Neo4J (and maybe some other databases) it won't really change something, since AnyVal were not usable at all.

@RustedBones
Copy link
Contributor

Other modules actually have the same behavior, considering value classes as record.
I'd rather say that this is a bug in all cases (no one would use value classes due to that in any module)

@turb
Copy link
Contributor Author

turb commented Oct 11, 2022

@RustedBones does it mean the PR will have to wait until all cases support it?

@RustedBones
Copy link
Contributor

I'm preparing an update for that. I'm almost done. Would it be fine to continue on your branch?

@turb
Copy link
Contributor Author

turb commented Oct 11, 2022

I'm preparing an update for that. I'm almost done. Would it be fine to continue on your branch?

Sure!

@RustedBones
Copy link
Contributor

With this change, magnolia macro do not guarantee to return a record (value-class is made transparent in the target model).
We'll have to relax the typeclass creation from the implicit, and check at runtime if it is a record

@turb
Copy link
Contributor Author

turb commented Oct 13, 2022

Note: Scala 2.13.9 broke binary compatibility on case classes that are value classes, so any update on it should go directly from Scala 2.13.8 to 2.13.10. See scala/scala#10155

@RustedBones
Copy link
Contributor

Thanks for the heads-up @turb . Next PR #615 will go to 2.13.10 directly

@RustedBones RustedBones merged commit 868096a into spotify:main Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants