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

Query builders refactoring / rewrite #2973

Merged
merged 6 commits into from
Jun 14, 2024
Merged

Query builders refactoring / rewrite #2973

merged 6 commits into from
Jun 14, 2024

Conversation

dstepanov
Copy link
Contributor

Rewrite of QueryBuilder to eliminate the conversion to the old criteria model; this will allow us to make modifications to the query builder much faster, allowing us to add more features like functions, etc.

Previous query builders were kept unmodified and replaced with QueryBuilder2, SqlQueryBuilder2 internally.

There are some small changes in how queries are generated. (IN literal values are now generated already expanded instead of doing it at the runtime).

In Micronaut 5 v2 implementation will replace the old builders.

Copy link

sonarcloud bot commented Jun 7, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
31.1% Duplication on New Code (required ≤ 10%)
4 New Bugs (required ≤ 0)
40 New Critical Issues (required ≤ 0)
1 New Blocker Issues (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Copy link
Contributor

@graemerocher graemerocher left a comment

Choose a reason for hiding this comment

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

Overall seems fine, but if we are going to rewrite this hierarchy I would like to see a model where we can support multiple dialects instead of just one like we have today.

It would also be nice if the dialect could be configured at the annotation processor argument level

* @since 3.9.0
*/
@Internal
public final class CosmosSqlQueryBuilder2 extends SqlQueryBuilder2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename this hierarchy LegacySqlQueryBuilder, adding the number 2 is confusing IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case it would be CosmosSqlQueryBuilderNext

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But that's only a temporary name till v5

@dstepanov
Copy link
Contributor Author

Overall seems fine, but if we are going to rewrite this hierarchy I would like to see a model where we can support multiple dialects instead of just one like we have today.

It would also be nice if the dialect could be configured at the annotation processor argument level

What do you mean by multiple dialects? SQL query builder can be instantiated with a different dialect, which can come from the annotation metadata

@graemerocher
Copy link
Contributor

What I would like is:

tasks.withType(JavaCompile).configureEach {
    options.compilerArgs.add("-Amicronaut.data.dialects=ORACLE,H2")
}
@JdbcRepository // no dialect
interface BookRepository extend CrudRepository<Book, Long> {}
datasources.default.dialect=H2 # use h2
@Inject BookRepository bookRepository; // receive H2 repository

@dstepanov dstepanov merged commit bf6a7a9 into 4.9.x Jun 14, 2024
50 of 51 checks passed
@dstepanov dstepanov deleted the builref branch June 14, 2024 06:37
@dstepanov
Copy link
Contributor Author

I don't see a big problem with supporting it. We would need to execute the query builder a few times and store multiple results.

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.

2 participants