-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add MongoDB with Panache extension #3650
Conversation
@emmanuelbernard, @FroMage : thanks the works we done together on the design (and for the paser ) |
Maybe it should be squashed? Or at least reduced to a number of important semantic commits? |
@gmset: of course, but as it's a big PR I propose to wait for a first round of feedback/updates before doing the cosmetic changes as I assume I will have to make a lot of changes anyway ;). |
63c7748
to
6602c01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@loicmathieu really great PR, I mostly read and reviewed the guide doc and I have a couple of rephrasing suggestions. Thanks.
|
||
MongoDB with Panacge relies on compile-time bytecode enhancements to your entities. | ||
If you define your entities in the same project where you build your Quarkus application, everything will work fine. | ||
If the entities come from external projects or jars, you can make sure that your jar is treated like a Quarkus application library by adding an empty `META-INF/beans.xml` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this #3545 lands, we may need a link to it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the cited PR has landed, maybe we can add a reference to How to Generate a Jandex Index
section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. If the phrasing is OK I can update the hibernate guide with the same one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsmet is the new phrasing OK? Should I update the hibernate guide with it ?
...ongodb-panache/runtime/src/main/java/io/quarkus/mongodb/panache/runtime/MongoOperations.java
Outdated
Show resolved
Hide resolved
...ongodb-panache/runtime/src/main/java/io/quarkus/mongodb/panache/runtime/MongoOperations.java
Outdated
Show resolved
Hide resolved
...ongodb-panache/runtime/src/main/java/io/quarkus/mongodb/panache/runtime/MongoOperations.java
Outdated
Show resolved
Hide resolved
67317cf
to
8db1717
Compare
@machi1990 can you review your conversations and close them if everything is OK for you |
I think the PR needs to be rebased onto |
8db1717
to
0dbc935
Compare
@geoand I rebase onto master, wait & see :) |
Cool! |
bc7ae00
to
c8edaad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't reviewed the code but I have made a first pass on the documentation.
return JsonbBuilder.create(config); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit unfortunate that you have to manually add this for the default id behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it's on the TODO list on the description of the PR :)
But the discussion about how to know if an extension is included needs to more forward because today there is no way to know if JsonB is included (not all extension provides a Capabilities - in fact only Arc and Narayana, so therer is currently no way to know that an extension is included).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, just add a capability if you need one. You can do that in separate PRs, we will merge them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, it's more complex that I first thought ...
Moreover the PR #3553, if merged, will change the way serializers/deserializers will be managed.
For the moment, I will update the TODO list with to do it later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsmet both JSON-B and Jackson serializer/deserializer are automatically registered if resteasy-jsonb or resteasy-jackson capability is present.
When changing one of these two library we need to carefully review this as it may creates incompabilities (for example an impact analyses needs to be done for PR #3553, and if we add some customization to Jackson ObjectMapper we need to port them to panache-mongo or change the way the ObjectMapper is created).
Fo this to work, I had to create a resteasy-common-spi
module.
we would never do something like that for regular objects in the Object Oriented architecture, where state and methods are in the same class. Moreover, this requires two classes per entity, and requires injection of the DAO or Repository where you need to do entity operations, which breaks your edit flow and requires you to get out of the code you're writing to set up an injection point before coming back to use it. | ||
- MongoDB queries are super powerful, but overly verbose for common operations, requiring you to write queries even | ||
when you don't need all the parts. | ||
- MongoDB queries are JSON based, so you will need some String manipulation or using the `Document` type and it will needs a lot of boilerplate code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the first reasons fly for MongoDB. They are more related to traditional JPA development than to the MongoDB API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can rephrase to
Even if MongoDB queries are not too verbose, PanacheQL go one step further by allowing to not write any queries for basic use cases.
This is a reference to find("name", name) shortcut query.
|
||
MongoDB with Panacge relies on compile-time bytecode enhancements to your entities. | ||
If you define your entities in the same project where you build your Quarkus application, everything will work fine. | ||
If the entities come from external projects or jars, you can make sure that your jar is treated like a Quarkus application library by adding an empty `META-INF/beans.xml` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please do that.
81d9123
to
de8b33d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry my review took so long, but that was a lot of code. Overall you know this is a great PR and we want to merge it ASAP. I added change requests, but those are pretty minor points.
Thanks a lot!
docs/src/main/asciidoc/index.adoc
Outdated
@@ -47,6 +47,7 @@ include::quarkus-intro.adoc[tag=intro] | |||
* link:kogito-guide.html[Using Kogito (business automation with processes and rules)] | |||
* link:oauth2-guide.html[Using OAuth2 RBAC] | |||
* link:tika-guide.html[Using Apache Tika] | |||
* link:mongodb-panache-guid.adoc[Simplified MongoDB with Panache] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest to put it next to the mongo guide, but it's not in the list :( Can you add it just before yours?
Also we need to figure out how to call the guide, because the current one is mongo-guide.adoc
and yours uses mongodb
. Given that the extension is called mongodb
I suggest we rename the current guide. Agreed @cescoffier ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I add it but I didn't rename it as it implies changing the links on the website and some coordination for the release. @gsmet is it worth it to rename the guide file name mongo-guide.adoc
to mongodb-guide.adoc
?
...b-panache/runtime/src/main/java/io/quarkus/mongodb/panache/runtime/PanacheQlQueryBinder.java
Show resolved
Hide resolved
...b-panache/runtime/src/main/java/io/quarkus/mongodb/panache/runtime/PanacheQlQueryBinder.java
Show resolved
Hide resolved
...ation-tests/mongodb-panache/src/main/java/io/quarkus/it/mongodb/panache/book/BookEntity.java
Show resolved
Hide resolved
...sts/mongodb-panache/src/main/java/io/quarkus/it/mongodb/panache/book/BookEntityResource.java
Outdated
Show resolved
Hide resolved
...ployment/src/main/java/io/quarkus/mongodb/panache/deployment/PanacheMongoEntityEnhancer.java
Show resolved
Hide resolved
2f6d94a
to
a753d02
Compare
Thanks @FroMage for your review. For me, there is two points that needs to be solved before being able to merge:
I need guidance for these two points, my opinion is to keep only I have stilll some tests to do (jackson and documentation among others), but for the other open points I prefere to open followup issues (as answered in my comments) when this PR will be merged as it's already a big PR opened since a long time :) |
I can see Stef hates the term upsert but that’s an industry accepted one.
persistOrUpdate is cool for me as well and I guess we could implement it on
data store that don’t support proper upserts.
…On Tue 17 Sep 2019 at 01:43, Stéphane Épardaud ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
extensions/panache/mongodb-panache/runtime/src/main/java/io/quarkus/mongodb/panache/PanacheMongoEntityBase.java
<#3650 (comment)>:
> + * @see PanacheMongoEntity
+ */
+public abstract class PanacheMongoEntityBase {
+
+ // Operations
+
+ /**
+ * Insert this entity in the database.
+ * This will set it's ID field if not already set.
+ *
+ * @see #insert(Iterable)
+ * @see #insert(Stream)
+ * @see #insert(Object, Object...)
+ * TODO delete and keep only persist ?
+ */
+ public void insert() {
OK. I think that at some point we have to accept that there's an idiomatic
way to do things when it comes to SQL people that differs from Mongo users.
SQL people will be used to insert/update being separated, especially if
coming from ORM (even though the update is implicit), and Mongo users will
complain if they use upsert all the time and we don't provide it.
I guess we can accept that we can please everyone and provide:
- persist (inserts and new entity, will complain if the entity is
already persisted or if the ID already exists in the DB)
- update (updates an already-persisted entity to the DB, will complain
if the entity is not in the DB)
- persistOrUpdate that does an upsert. We can't use save next to
persist because it's just a guess how they differ without reading the
docs. upsert() is kind of lame because we're using persist and not
insert, and upsist is gabbleduck. saveOrUpdate would be admitting that
we should have called persist save ;)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3650?email_source=notifications&email_token=AACJNWEV27IELCNMWNRQRIDQKCKCXA5CNFSM4IOVIEIKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCE5WJSA#discussion_r325047627>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACJNWFHMP7VLM4UPVGZACTQKCKCXANCNFSM4IOVIEIA>
.
|
I don't hate |
@loicmathieu you're good to go on my end. The native test failure doesn't seem related. Did you rebase on the latest master? |
703528b
to
d48d173
Compare
@FroMage thanks a lot :) |
Hah, it passed CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a bunch of comments and questions.
I would appreciate additional commit(s) for the updates, then I think we should squash everything into only one atomic commit.
|
||
import io.quarkus.test.junit.SubstrateTest; | ||
|
||
@DisabledOnOs(OS.WINDOWS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need a comment describing why it's not run on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It nows run again on Windows
|
||
@Container | ||
private static final GenericContainer MONGO = new FixedHostPortGenericContainer("mongo:4.0") | ||
.withFixedExposedPort(27018, 27017); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we use test containers? We already use them for the other MongoDB extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use test containers because mongodb-embedded that was use in the mongodb-client extension causes issue on my laptop (need to run the test as root). Other extension would also want to use test containers because it's very conveniant (Vault extension), there is an issue opened about it.
IMHO leveraging test containers can be very conveniant for Quarkus integration tests (for example to test different DBs).
But now that I finish the implementation, I can switch back to embeded MongoDB as it is used by mongodb-client extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I go back to flapddodle for embedded mongo to be able to run in Windows
import io.restassured.parsing.Parser; | ||
import io.restassured.response.Response; | ||
|
||
@DisabledOnOs(OS.WINDOWS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a comment explaining why Windows is not supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It now runs again on Windows
quarkus.mongodb.write-concern.journal=false | ||
quarkus.mongodb.database=books | ||
|
||
quarkus.log.category."io.quarkus.mongodb.panache.runtime".level=DEBUG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove that to avoid having the build being too verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log DEBUG on panache mongo only allow to trace the queries send to MongoDB. For the current integration test it adds 10 lines to the buid.
I found it very convenient, is it OK to keep it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general rule, I would prefer to have it commented out but I suppose we can live with 10 more lines :).
</configuration> | ||
</plugin> | ||
<plugin> | ||
<groupId>${project.groupId}</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to make that one io.quarkus
now or you will have a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
== Transactions | ||
|
||
MongoDB offers ACID transactions since version 4.0. MongoDB with Panache doesn't provide support for them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe make it a warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
I hope it can be done soon as Narayana should be able to provide support for it
e0c8442
to
4bd0d3b
Compare
@gsmet I finally found the issue with this PR, I made a bad merge ... |
d561d4a
to
787db9e
Compare
@loicmathieu yes please squash everything and, unfortunately, we have a conflict now. I'll try to merge it as soon as CI is green. |
787db9e
to
1946498
Compare
@gsmet I rebased, fixed the merge issue (conflicted build capabilities), launched the test and squashed everything. |
Excited to see this in. Thanks @loicmathieu. |
@machi1990 as it is a big PR, testing and feedback will be appreciate :) |
@gsmet: all green !!! |
Merged! Congrats to you and Stéphane for this big achievement. |
Thanks for all you guys that worked with me on this one: @FroMage, @emmanuelbernard and @gsmet it's been a long and very interresting journey :) |
This PR adds a new extension: MongoDB with Panache.
This is a Panache implementation for MongoDB inspired by the one for Hibernate.
It provides the following:
@BsonId
,@BsonIgnore
and@BsonProperty
.@MongoEntity
annotation to customize the name of the collection and the database.Person.find("{'firstname':?1}", firstname)
orPerson.find("{lastname'::lastname}", lastname)
Person.find("firstname", firstname)
orPerson.find("firstname = ?1 and lastname = ?2", ...)
orPerson.find("firstname =:firstname 1 and lastname = :lastname", ...)
, you can find more query example on the guide with the list of operators supported by PanacheQL.@BsonProperty
are only supported on PanacheQL queries not on native one.project()
method on PanacheQuery :Person.find("firstname", firstname).project("lastname)
ObjectId
, the name of the field is 'id' and can be customized via@BsonId
. I create serializer/deserializer to String to facilitate it's usage with resteasy but didn't include them by default.Document
querygetMongoCollection()
andgetMongoDatabase()
to access the underlying database and collection.There is still some works to be done:
getMongoCollection
andgetMongoDatabase
that both needs to pass the entity class as parameter.ObjectId
serializer/deserializer when resteasy-jsonb is in the classpathObjectId
and include them whenresteasy-jacson is in the classpath