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

Updated MongoDB/Panache parser to latest ORM parser #38751

Merged
merged 2 commits into from
Feb 14, 2024

Conversation

FroMage
Copy link
Member

@FroMage FroMage commented Feb 13, 2024

This way, we keep it up to date, because the previous version was a pre-release of ORM 5-6.
This required some visitor changes, and in particular there's a precedence change between 'and/or' expresions who now require paren grouping.
Also, literals are not implemented correctly: only strings are supported but this was already the case: tests do not test other literals.

I could not run the tests locally, due to an outdated libssl requirement of flapdoodle. Hopefully CI will get it running.

Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

LGTM.

For primitive types, I'm surprised it didn't worked and nobody complained. We should add tests for them and see what happens, we can add them later.

@FroMage
Copy link
Member Author

FroMage commented Feb 13, 2024

Caused by: com.mongodb.MongoWriteException: Write operation error on server localhost:37017. Write error: WriteError{code=11000, message='E11000 duplicate key error collection: test.Book index: _id_ dup key: { _id: "idiot" }', details={}}.

Not sure if this is my fault, I guess it must be. Do you have any idea how to run these tests locally on recent machines? Ones without old libssl versions?

This comment has been minimized.

@loicmathieu
Copy link
Contributor

Tests runs on my machine so I cannot help you on that.
But to troubleshoot query issues you can enable query debugging: https://quarkus.io/guides/mongodb-panache#query-debugging

@FroMage
Copy link
Member Author

FroMage commented Feb 14, 2024

I suppose it's due to something like this? flapdoodle-oss/de.flapdoodle.embed.mongo#451

@loicmathieu
Copy link
Contributor

It may.

To test you can also launch locally and use testcontainers or start a MongoDB via Docker.
It may not be that simple as I think you must start it twice for all tests to works.

@FroMage
Copy link
Member Author

FroMage commented Feb 14, 2024

Looks like MongoTestResource defaults to V4_4(V4_4_18), and the issue linked says that I need V6 to support the latest Ubuntu.

@loicmathieu
Copy link
Contributor

You can switch locally to 6 by updating the @TestResource annotation, it allows passing the version.

@FroMage
Copy link
Member Author

FroMage commented Feb 14, 2024

Yeah, except the @QuarkusTestResource is global to the entire module, so I need to update them all, or move them to a single class.

But it works, I can reproduce now.

@FroMage
Copy link
Member Author

FroMage commented Feb 14, 2024

So, we have a query of the form instant < ?2 and instant is now a function in HQL, which returns the current instant.

We have several choices here, like ask people to quote those identifiers/functions if they want to use them as attributes: 'instant' < ?2, which allows us to implement these functions later.

Or we can declare we'll never use them, and turn these functions into identifiers, as was the case before.

WDYT?

@FroMage
Copy link
Member Author

FroMage commented Feb 14, 2024

Actually, I see QUOTED_IDENTIFIER in the grammar, which is `instant` but I'm not sure that's really it.

@FroMage
Copy link
Member Author

FroMage commented Feb 14, 2024

A quoted identifier is written in backticks. Quoting lets you use a keyword as an identifier.

select thing.interval.`from` from Thing thing
Actually, in most contexts, HQL keywords are "soft", and don’t need to be quoted. The parser is usually able to distinguish if the reserved word is being used as a keyword or as an identifier.

So it is backticks.

@loicmathieu
Copy link
Contributor

I'm afraid that if we ask users to quote what was not needed before it may break existing code. If we can avoid that it will be better as it's anyway useless for MongoDB where all functions starts with $.

@FroMage
Copy link
Member Author

FroMage commented Feb 14, 2024

OK, let me remove those keywords then. They do seem useful, though, given that they provide access to the current time. But whatever, not my problem ;)

This way, we keep it up to date, because the previous version was a
pre-release of ORM 5-6.
This required some visitor changes, and in particular there's a
precedence change between 'and/or' expresions who now require paren
grouping.
Also, literals are not implemented correctly: only strings are supported
but this was already the case: tests do not test other literals.
Because they are global and can only be configured at one place
@FroMage FroMage force-pushed the mongo-parser-update branch from ae57329 to d19c93b Compare February 14, 2024 14:42
@FroMage
Copy link
Member Author

FroMage commented Feb 14, 2024

I did fix this bug, but tests still fail, though I suspect this may be due to mongo V6 because I don't see any query issue. So let's see what CI says.

Copy link

quarkus-bot bot commented Feb 14, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit d19c93b.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@FroMage
Copy link
Member Author

FroMage commented Feb 14, 2024

Well, CI says it's fine for V4. I should probably open an issue to see why the tests don't run in V6, no?

@FroMage FroMage merged commit 06eaa39 into quarkusio:main Feb 14, 2024
20 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Feb 14, 2024
@FroMage FroMage deleted the mongo-parser-update branch February 14, 2024 16:22
@loicmathieu
Copy link
Contributor

I should probably open an issue to see why the tests don't run in V6, no?

I would say wait for someone to complain 🙈

But yes, something may have changed

@FroMage
Copy link
Member Author

FroMage commented Feb 15, 2024

🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants