implement kotlin support for mongodb panache #11043

evanchooly · 2020-07-28T20:58:25Z

There were two goals, as we discussed on zulip at the outset, of this PR:

Implement the same support for Kotlin in MongoDB Panache as we did for Hibernate Panache.
Try to push for greater reuse of code to reduce the amount of duplication

So there are some changes in here not strictly necessary for #1 but moves the ball down the field for #2. That said, I didn't go all the way with the deduplication because it was taking a fair bit of time and I wanted to get the functionality in first. Further dedeuplication would be mostly behind the scenes clean up. However, i would like to point out how the number of visitors we have here vs the JPA version is noticably lower. I think we can go even further, to be honest, but I wanted to checkpoint what I have here and make sure everyone's on board before going further.

These changes are largely limited to the MongoDB code but some did leak up in to the JPA and common areas because $reasons. I've tried to limit that leakage and in some ways it dilutes the value of the deduplication effort but I'm reasonably happy with what I did get done and think there's value in going further. There's a few options I'd like to explore in sharing interfaces for things like PanacheQuery and such that could further reduce the amount of duplication and move to more centralized, shared code but those cuts are likely to be deeper/more widespread and I want to get validation first.

I'm happy to set up a bluejeans chat to walk through the PR if you'd like or you're welcome to review them on your own. Whichever is easiest for you. I know there are some changes in there that might raise more questions than others and I'm happy to share my thinking on those. Hopefully you'll agree with them.

loicmathieu · 2020-07-29T07:35:01Z

@evanchooly this is indeed a big PR and I already need to have a look at the big Hibernate with Panache Reactive PR so it may be difficult to merge this for 1.7 CR1 that should be out tomorrow.

Anyway, this looks very great and I'll be happy to review it soon, and yes a quick chat to explain it would be great ! I don't know if @FroMage want to be part of this chat ?

There's a few options I'd like to explore in sharing interfaces for things like PanacheQuery and such that could further reduce the amount of duplication and move to more centralized, shared code

This maked me smile as I'm agree with you but during the design of MongoDB with Panache it has been decided to not share anything (except some minor stuff like Page) on the API side between MongoDB and Hibernate. So we make our best to keep the same API (for Entity, Repository and PanacheQuery) but no supertype has been implemented.

If I remember well, the concern was that MongoDB and Hibernate (and possibly other technology if we decided to create a Panache implementation for another database technology later) are very different in nature (RDMBS / NSQL, session base / no session, ORM / POJO, SQL / Document query, ...) and that providing common types could be limiting and would introduce coupling.

I do think that on the PanacheQuery side we could have introduce a Findable interface with the common parts but this has been dismissed ...

gsmet · 2020-07-29T09:26:01Z

Yes, that's too big for last minute merge. This will have to wait for 1.8.

evanchooly · 2020-07-29T14:15:12Z

@loicmathieu i wasn't thinking sharing between hibernate and jpa so much, actually. i think those models are different enough that it would be awkward at best to share dev-facing APIs between them. I was thinking mostly in having one PanacheQuery in, say, mongodb-panache shared between the java and kotlin sides. Same for hibernate. In the hibernate side it was generics and nullability that were the biggest blockers on that front but I think much of that can be worked around.

That said, there's a lot of common code in the visitors that can be centralized. Again, we may only be able to centralize up to the hibernate/mongodb divide but that's still a fairly big win. I started some of that in this PR just to see what's possible but I think we can get more out of that process still.

evanchooly · 2020-07-29T18:48:27Z

rebased against the update to mongo client PR

loicmathieu

This is really great work! Cool to see Kotlin support lands in MongoDB with Panache ;)

I have some general questions:

PanacheMongoEntityBase: MongoOperations is defined as a companion object also used inside the repository. This seems strange to me.
What's the purpose of KotlinMongoOperations ?
Why are PanacheQueryImpl and PanacheUpdateImpl duplicated ? This was not needed for the Hibernate extension.
I have some big concerns about the change to MongoOperations from a static helper class to a class instanciated into a static field inside entity / repository. This is a breaking change in case some people directly use MongoOperations. I know that nobody should have done this but I saw some strange usage on Stack Overflow or Zulip ...

I quickly go throught the deploymenent part, it seems OK for me but I really want @FroMage takes on this.

...e/mongodb-panache-common/runtime/src/main/java/io/quarkus/mongodb/panache/PanacheUpdate.java

loicmathieu · 2020-07-30T11:09:21Z

...e-kotlin/runtime/src/main/kotlin/io/quarkus/mongodb/panache/kotlin/PanacheMongoEntityBase.kt

+     * @see .persist
+     * @see .persist
+     * @see .persist


Why is there multiple @see for .persist.
Same for update, delete, ...

These are weird artifacts of the conversion to kotlin. I'll need to rewrite them in dokka. But if I can pull off that common interface definition between java and kotlin, this would all go away.

loicmathieu · 2020-07-30T11:11:45Z

...tlin/runtime/src/main/kotlin/io/quarkus/mongodb/panache/kotlin/PanacheMongoRepositoryBase.kt

+     * @see .update
+     * @see .update
+     * @see .update


Same here, the @see tags seems not correct ...

...he-kotlin/runtime/src/main/kotlin/io/quarkus/mongodb/panache/kotlin/PanacheMongoCompanion.kt

...main/kotlin/io/quarkus/mongodb/panache/kotlin/reactive/ReactivePanacheMongoRepositoryBase.kt

...src/main/kotlin/io/quarkus/mongodb/panache/kotlin/reactive/ReactivePanacheMongoEntityBase.kt

integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/BookResource.java

integration-tests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/JacksonTest.java

...godb-panache-kotlin/src/main/kotlin/io/quarkus/it/mongodb/panache/book/BookEntityResource.kt

loicmathieu · 2020-07-30T12:41:33Z

...db-panache-kotlin/src/test/kotlin/io/quarkus/it/mongodb/panache/MongodbPanacheMockingTest.kt

+    // These mocks are trying to call methods against instances but kotlin doesn't allow these kinds of method calls.
+    // they must be called against the companion object (invoked against the type not a reference).  removing these tests
+    // for now as I neither know how to correct this at this point nor am I convinced of the utility of these tests.


PanacheMock provides the capability to mock entity static methods.
If the test can be done by mocking on the companion object, cool, if not, it should be documented somewhere that we cannot mock entity static methods in Kotlin

This is what we discussed last week. I have a better understanding of how the mocking stuff is set up now so I can take another shot at enabling this before it goes out in a release.

evanchooly · 2020-08-04T14:42:22Z

rebased again with master. found some bugs my testing script managed to hide to in all the scrolling.

full re-review requested with updated PR

loicmathieu

Overall it looks good, it complicates some parts on the runtime module (I will need to investiguate more time to be used to it) but as you explain, we don't have a lot of choice and it solves a lot of issue.

For the deployment part, I do love what you did with the common classes, it is easily understandable. But again, I will need @FroMage takes on this.

My main concern is that PanacheQueryImpl and PanacheUpdateImpl are duplicated.
This was not needed for the Hibernate extension so I wonder if it's really needed here.

evanchooly · 2020-08-05T14:18:18Z

There actually is a PanacheQuery on the hibernate side. Although I think there's a chance this could be cleared up. I think the duplication is due to how generics works a bit differently on the kotlin side. But i'll try a spike with reusing those interfaces here and see how it goes. If we can reduce more duplication, I'm all for it.

loicmathieu · 2020-08-05T16:13:53Z

@evanchooly

There actually is a PanacheQuery on the hibernate side

Yes but there is no PanacheQueryImpl. I'm OK to have duplicated interfaces to customize the return type but I'm afraid that having duplicated implementations creates a lot of maintainance.

evanchooly · 2020-08-05T16:35:23Z

There actually is:

quarkus/extensions/panache/hibernate-orm-panache-kotlin/runtime/src/main/java/io/quarkus/hibernate/orm/panache/kotlin/runtime/PanacheQueryImpl.java

Line 30 in 8013545

// Builder

It comes down, as I suspected, to generics. What is different is that on the hibernate side it's a modified copy of the java version where the mongo side currently has the impl converted to kotlin. I don't mind going the same route as the hibernate impl for simplicity but I'm not sure it buys us that much. The java version is still modified to account for the generics but it's a much more cosmetic change. I'll rework to do the same for all the Impl classes as that at least removes a hurdle or two for maintenance.

loicmathieu · 2020-08-06T07:13:12Z

@evanchooly

What is different is that on the hibernate side it's a modified copy of the java version where the mongo side currently has the impl converted to kotlin

As I understand it on Hibernate, both Java and Kotlin PanacheQueryImpl uses a CommonPanacheQuery and delegate all calls to it but on the MongoDB side there is two implementations. So the code is duplicated on MongoDB but on HIbernate it's just the API side that is duplicated.

FroMage

Looks good, I only have minor comments, except the requirement for a shared query impl like we do for ORM.

FroMage · 2020-08-06T14:13:16Z

core/deployment/src/main/java/io/quarkus/deployment/ExtensionLoader.java

+    protected static List<Method> getMethods(Class<?> clazz) {
+        List<Method> declaredMethods = new ArrayList<>();
+        if (!clazz.getName().equals(Object.class.getName())) {
+            declaredMethods.addAll(getMethods(clazz.getSuperclass()));


Generally I prefer to split those sort of methods in two, with one taking a mutable List result parameter to avoid creating one collection per class just to throw it away in addAll.
Also, won't this behaviour change impact other users?

I generally don't, obviously, but i'm happy to break that up if you want. It doesn't seem to affect anyone else as it would only affect someone who'd defined BuildStep on a class with a parent which doesn't seem to be happening anywhere else. All the other tests I've run still pass, though.

FroMage · 2020-08-06T14:14:55Z

core/deployment/src/main/java/io/quarkus/deployment/util/AsmUtil.java

+        }
+    }
+
+    public static org.objectweb.asm.Type autobox(org.objectweb.asm.Type primitive) {


Not sure who calls this, but you can't box Void, so that's dodgy.

It would actually convert void to Void but in this case that can't happen as it's only applied to method parameters in this case. It's called here.

FroMage · 2020-08-06T14:23:27Z

...java/io/quarkus/hibernate/orm/panache/kotlin/deployment/KotlinPanacheEntityClassVisitor.java

-
-    @Override
-    protected String getPanacheOperationsBinaryName() {
+    protected String getPanacheOperationsInternalName() {


This introduces confusion wrt what is an internal vs binary name, why?

I would actually like to entirely eliminate the use of "binary name" as that does not appear (that I can find) in either asm or jandex but appears to have been crafted with quarkus. Usually the use of the different forms of these name is either as a descriptor (L/java/lang/String;) or an internal name (java/lang/String). But making that change now is far reaching and I didn't want to unilaterally make that decision. I think only changed that in this case because I kept getting confused as to what that form should in this case. But the formal/official term is "internal name" at any rate.

FroMage · 2020-08-06T14:26:51Z

...t/src/main/java/io/quarkus/mongodb/panache/deployment/BasePanacheMongoResourceProcessor.java

+    public static final String OBJECT_SIGNATURE = toBinarySignature(Object.class);
+    public static final DotName PROJECTION_FOR = createSimple(ProjectionFor.class.getName());
+
+    protected static String toBinarySignature(Class<?> type) {


This is confusing because descriptors and signatures are different, and this seems to confuse the two.

Yeah. I can rename it to toDescriptor() but I'm actually hoping to change the type to a ByteCodeType as i feel that's a bit of cleaner type than a raw String. Makes it easier to under stand in context where it's used.

...kus/mongodb/panache/kotlin/deployment/visitors/KotlinPanacheMongoRepositoryClassVisitor.java

...c/main/java/io/quarkus/mongodb/panache/kotlin/reactive/runtime/ReactivePanacheQueryImpl.java

...src/main/kotlin/io/quarkus/mongodb/panache/kotlin/reactive/ReactivePanacheMongoEntityBase.kt

FroMage · 2020-08-06T15:00:21Z

...che/deployment/src/main/java/io/quarkus/mongodb/panache/deployment/ImperativeTypeBundle.java

+
+    @Override
+    public ByteCodeType entityBaseCompanion() {
+        throw new UnsupportedOperationException("Companions are not supported in Java.");


Feels weird to have this method defined in the Java base interface if it's only meant for Kotlin implementations. Is it called by the abstract processor? If not, it should probably belong in a Kotlin subinterface.

Well, the idea was to have a single interface. Generally the java side would never call that because it doesn't make sense but it means we can simply pass around TypeBundle references and not need to worry about subtypes. I had actually thought about bumping the definition of those methods to default methods and then the java implementations can simply ignore them. But I did have a case once early on when I was experimenting with this approach where I had passed in the wrong type and got the java type defs instead and this exception revealed that error quite quickly.

evanchooly · 2020-08-10T18:15:42Z

Apart from the documentation, are there any other issues to go over. I've been waiting for things to settle a bit before going through the work of updating the docs to dokka in case there were impactful changes to be made there. But it feels like we're close enough to a consensus to update the docs. I'll start working on those now but in the meantime it would be helpful to get a thumbs up/down on the technical bits before this PR ages any more.

loicmathieu

Now that the query implementation is shared, I'm very happy with this implementation so it's a big LGTM !

If I understand correctly, the TestAnalogs.kt is here to be sure that all methods of entity/repository/query of Java exist in the Kotlin version ? I love it !

evanchooly · 2020-08-11T20:33:48Z

@FroMage any other comments/questions?

evanchooly · 2020-08-12T18:15:01Z

rebased against master again to protect against drift

mostly outdated now

gsmet · 2020-08-18T09:00:07Z

I just reverted this PR. CI is all red: https://github.com/quarkusio/quarkus/pull/11043/checks and it broke the master branch.

Please check that CI has properly run before merging a PR.

evanchooly · 2020-08-18T16:54:39Z

My apologies. In my experience, master is always broken in some odd corner of the world and getting feedback into inquiries about those failures has proven to almost impossible. So I've trained myself to ignore those failures that are unrelated to my changes. If that is no longer the case, then I can safely assume i broke things if they fail locally. Since CI can (at the moment) be trusted again, then what I'll start doing is filing draft PRs to track my work as I go and let the CI runs happen overnight since they take forever to run. Then when I'm done with it all, I can be reasonably sure i haven't broken something in some random corner (like this case was). Then asking for a review will be much more productive.

evanchooly added this to the 1.7.0 - master milestone Jul 28, 2020

evanchooly requested review from FroMage and loicmathieu July 28, 2020 20:58

boring-cyborg bot added area/core area/dependencies Pull requests that update a dependency file area/mongodb area/panache labels Jul 28, 2020

gsmet removed this from the 1.7.0 - master milestone Jul 29, 2020

loicmathieu mentioned this pull request Jul 29, 2020

Fix MongoClient handling in Mongo Panache #11058

Merged

loicmathieu previously requested changes Jul 30, 2020

View reviewed changes

evanchooly requested a review from loicmathieu August 4, 2020 14:41

loicmathieu requested changes Aug 5, 2020

View reviewed changes

FroMage previously requested changes Aug 6, 2020

View reviewed changes

loicmathieu approved these changes Aug 11, 2020

View reviewed changes

implement kotlin support for mongodb panache

0adc1d3

evanchooly requested a review from FroMage August 17, 2020 17:48

evanchooly merged commit bdcb548 into quarkusio:master Aug 17, 2020

gsmet added the triage/invalid This doesn't seem right label Aug 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement kotlin support for mongodb panache #11043

implement kotlin support for mongodb panache #11043

evanchooly commented Jul 28, 2020

loicmathieu commented Jul 29, 2020

gsmet commented Jul 29, 2020

evanchooly commented Jul 29, 2020

evanchooly commented Jul 29, 2020

loicmathieu left a comment

loicmathieu Jul 30, 2020

evanchooly Jul 30, 2020

loicmathieu Jul 30, 2020

loicmathieu Jul 30, 2020

evanchooly Jul 30, 2020

evanchooly commented Aug 4, 2020

loicmathieu left a comment

evanchooly commented Aug 5, 2020

loicmathieu commented Aug 5, 2020

evanchooly commented Aug 5, 2020

loicmathieu commented Aug 6, 2020

FroMage left a comment

FroMage Aug 6, 2020

evanchooly Aug 6, 2020

FroMage Aug 6, 2020

evanchooly Aug 6, 2020

evanchooly Aug 6, 2020

FroMage Aug 6, 2020

evanchooly Aug 6, 2020

FroMage Aug 6, 2020

evanchooly Aug 6, 2020

FroMage Aug 6, 2020

evanchooly Aug 6, 2020

evanchooly commented Aug 10, 2020

loicmathieu left a comment •

edited

Loading

evanchooly commented Aug 11, 2020

evanchooly commented Aug 12, 2020

gsmet commented Aug 18, 2020

evanchooly commented Aug 18, 2020

implement kotlin support for mongodb panache #11043

implement kotlin support for mongodb panache #11043

Conversation

evanchooly commented Jul 28, 2020

loicmathieu commented Jul 29, 2020

gsmet commented Jul 29, 2020

evanchooly commented Jul 29, 2020

evanchooly commented Jul 29, 2020

loicmathieu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanchooly commented Aug 4, 2020

loicmathieu left a comment

Choose a reason for hiding this comment

evanchooly commented Aug 5, 2020

loicmathieu commented Aug 5, 2020

evanchooly commented Aug 5, 2020

loicmathieu commented Aug 6, 2020

FroMage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanchooly commented Aug 10, 2020

loicmathieu left a comment • edited Loading

Choose a reason for hiding this comment

evanchooly commented Aug 11, 2020

evanchooly commented Aug 12, 2020

gsmet commented Aug 18, 2020

evanchooly commented Aug 18, 2020

loicmathieu left a comment •

edited

Loading