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

Delegate projections with a select clause to ORM #38931

Merged
merged 2 commits into from
Jun 7, 2024
Merged

Conversation

FroMage
Copy link
Member

@FroMage FroMage commented Feb 21, 2024

Problem is that there's a bug in ORM that disallows projections of single columns, but that will be resolved by hibernate/hibernate-orm#7874

Fixes #31117

This comment has been minimized.

This comment has been minimized.

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.

You added //FIXME comment that seems more like a dev comment, or a TODO.
I'm not fan of having FIXME in the code, or maybe create an issue describing what needs to be fixed and add the link int the FIXME comment.

@FroMage
Copy link
Member Author

FroMage commented Feb 22, 2024

Looks like Kotlin is testing single field projections…

@FroMage
Copy link
Member Author

FroMage commented Feb 22, 2024

You added //FIXME comment that seems more like a dev comment, or a TODO.
I'm not fan of having FIXME in the code, or maybe create an issue describing what needs to be fixed and add the link int the FIXME comment.

Yeah, it's something that's always been there I suppose, but it stroke me as a bug, I thought I'd leave a note for the person who'd find the bug and want to fix it later.

@FroMage
Copy link
Member Author

FroMage commented Feb 22, 2024

I don't know what to do with this. This fixes reported bugs, but introduces a regression around single-column projections, which is fixed in ORM 6.5.0 (not released yet). Either we wait until ORM is released to merge this, or I merge it because single-column projections are not very common, or I merge this with a workaround for single-column projections until ORM is released.

Any opinion?

@loicmathieu
Copy link
Contributor

If ORM 6.5.0 is not too far away, it is better to wait for it than to introduce a regression.

@FroMage
Copy link
Member Author

FroMage commented Mar 1, 2024

If ORM 6.5.0 is not too far away, it is better to wait for it than to introduce a regression.

Fair enough.

@FroMage
Copy link
Member Author

FroMage commented Jun 5, 2024

Rebased on main now that we have ORM with the fix.

This comment has been minimized.

@FroMage
Copy link
Member Author

FroMage commented Jun 5, 2024

Well this is weird, it's supposed to be fixed :(

@FroMage
Copy link
Member Author

FroMage commented Jun 6, 2024

Oh, HR, let's see…

@FroMage
Copy link
Member Author

FroMage commented Jun 6, 2024

Ah…

2024-06-06 14:37:04,093 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (vert.x-eventloop-thread-2) HTTP Request to /test/projection2 failed, error id: 65e4acde-2a86-408c-8d01-fee35ce85544-1: java.lang.RuntimeException: Cannot count on more than one column

I suppose I can add support for that, but it's soon going to be supported by HR…

@FroMage
Copy link
Member Author

FroMage commented Jun 7, 2024

I can fix that, but then I'm left with Select item at position 1 in select list has no alias (aliases are required in CTEs and in subqueries occurring in from clause) which is caused by https://hibernate.atlassian.net/browse/HHH-18234 so it's like for #40962 we're blocked waitin for 6.5.3

FroMage added 2 commits June 7, 2024 11:37
Problem is that there's a bug in ORM that disallows projections of
single columns, but that will be resolved by hibernate/hibernate-orm#7874

Fixes quarkusio#31117
Using a subselect. Only useful for HR until they support count queries,
but it was easy enough to fix
@FroMage
Copy link
Member Author

FroMage commented Jun 7, 2024

OK, last try?

Copy link

quarkus-bot bot commented Jun 7, 2024

Status for workflow Quarkus CI

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

✅ 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.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 integration-tests/reactive-messaging-kafka

io.quarkus.it.kafka.KafkaConnectorTest.testFruits - History

  • Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
	at io.quarkus.it.kafka.KafkaConnectorTest.testFruits(KafkaConnectorTest.java:63)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)

@FroMage FroMage merged commit 8f9ed72 into quarkusio:main Jun 7, 2024
41 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone Jun 7, 2024
@FroMage FroMage deleted the 31117 branch June 7, 2024 13:39
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.

Exception in panache, when using newline before from
2 participants