codeconsole commented on code in PR #15962: URL: https://github.com/apache/grails-core/pull/15962#discussion_r3565184797
########## grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/WhereQueryEmbeddedSpec.groovy: ########## @@ -0,0 +1,348 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.data.testing.tck.tests + +import grails.gorm.DetachedCriteria + +import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec +import org.apache.grails.data.testing.tck.domains.ExternalRef +import org.apache.grails.data.testing.tck.domains.WorkItem +import org.apache.grails.data.testing.tck.domains.WorkItemGroup +import org.grails.datastore.mapping.query.Query +import org.grails.datastore.mapping.query.Restrictions +import spock.lang.Issue +import spock.lang.Requires + +/** + * Tests for where {} queries against properties of embedded components. + */ +class WhereQueryEmbeddedSpec extends GrailsDataTckSpec { + + void setupSpec() { + manager.registerDomainClasses(WorkItem, WorkItemGroup) + } + + private void createWorkItems() { + new WorkItem(description: 'first', extRef1: new ExternalRef(provider: 'SAP', value: 'ABC-123')).save(flush: true, failOnError: true) + new WorkItem(description: 'second', extRef1: new ExternalRef(provider: 'Jira', value: 'XYZ-456')).save(flush: true, failOnError: true) + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'where query with like on inherited embedded component property'() { + given: + createWorkItems() + + when: + String search = 'ABC' + def query = WorkItem.where {} + query = query.where { + extRef1.value =~ "%${search}%" + } + def results = query.list() + + then: + results.size() == 1 + results[0].description == 'first' + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'where query with equals on inherited embedded component property'() { + given: + createWorkItems() + + when: + def results = WorkItem.where { + extRef1.provider == 'SAP' + }.list() + + then: + results.size() == 1 + results[0].description == 'first' + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'where query with conjunction on embedded component property'() { + given: + createWorkItems() + + when: + def results = WorkItem.where { + description == 'first' && extRef1.value =~ '%ABC%' + }.list() + + then: + results.size() == 1 + results[0].description == 'first' + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'where query with disjunction on embedded component property'() { + given: + createWorkItems() + + when: + def results = WorkItem.where { + description == 'none' || extRef1.provider == 'SAP' + }.list() + + then: + results.size() == 1 + results[0].description == 'first' + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'where query with multiple dotted predicates on the same embedded component'() { + given: + createWorkItems() + + when: + def results = WorkItem.where { + extRef1.provider == 'SAP' && extRef1.value =~ '%ABC%' + }.list() + + then: + results.size() == 1 + results[0].description == 'first' + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'where query with an embedded component predicate in a nested junction'() { + given: + createWorkItems() + + when: + def results = WorkItem.where { + (description == 'none' || extRef1.provider == 'Jira') && description == 'second' + }.list() + + then: + results.size() == 1 + results[0].description == 'second' + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'where query with negated embedded component predicate'() { + given: + createWorkItems() + + when: + def results = WorkItem.where { + !(extRef1.provider == 'SAP') + }.list() + + then: + results.size() == 1 + results[0].description == 'second' + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'where query with inList on embedded component property'() { + given: + createWorkItems() + + when: + def results = WorkItem.where { + extRef1.provider in ['SAP', 'Oracle'] && description != 'none' + }.list() + + then: + results.size() == 1 + results[0].description == 'first' + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'where query with an embedded association block'() { + given: + createWorkItems() + + when: 'the embedded component is addressed with an association block, with an outer condition matching both items' + def results = WorkItem.where { + description != 'none' && extRef1 { provider == 'SAP' } + }.list() + + then: + results.size() == 1 + results[0].description == 'first' + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'count on a where query with an embedded component predicate in a junction'() { + given: + createWorkItems() + + when: 'the queries are built outside the assertion so the where DSL transformation applies' + def conjunctionQuery = WorkItem.where { + description == 'first' && extRef1.value =~ '%ABC%' + } + def disjunctionQuery = WorkItem.where { + description == 'none' || extRef1.provider == 'Jira' + } + + then: + conjunctionQuery.count() == 1 + disjunctionQuery.count() == 1 + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'criteria query with an embedded association block inside a disjunction'() { + given: + createWorkItems() + + when: 'the embedded block is nested in a junction, translating through AssociationQuery' + def results = WorkItem.createCriteria().list { + or { + eq('description', 'none') + extRef1 { + eq('provider', 'SAP') + } + } + } + + then: + results.size() == 1 + results[0].description == 'first' + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'where query with a disjunction inside an embedded association block'() { + given: + createWorkItems() + + when: 'the embedded block itself contains a disjunction' + def results = WorkItem.where { + extRef1 { provider == 'Oracle' || value =~ '%ABC%' } + }.list() + + then: + results.size() == 1 + results[0].description == 'first' + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'criteria query with a disjunction inside an embedded association block'() { + given: + createWorkItems() + + when: 'the embedded block itself contains a disjunction' + def results = WorkItem.createCriteria().list { + extRef1 { + or { + eq('provider', 'Oracle') + like('value', '%ABC%') + } + } + } + + then: + results.size() == 1 + results[0].description == 'first' + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'criteria query with an embedded association block inside a conjunction'() { + given: + createWorkItems() + + when: 'the embedded block itself carries more than one predicate' + def results = WorkItem.createCriteria().list { + and { + like('description', 'fir%') + extRef1 { + eq('provider', 'SAP') + like('value', '%ABC%') + } + } + } + + then: + results.size() == 1 + results[0].description == 'first' + } + + @Issue('https://github.com/apache/grails-core/issues/15955') + void 'where query with a disjunction inside an embedded association block'() { Review Comment: This feature method has the same name as the one at line 223 (`'where query with a disjunction inside an embedded association block'`). It only compiles because Spock renames feature methods before Groovy's duplicate-signature check, but both tests report under an identical display name, so a failure in either is ambiguous in test reports. This one combines the embedded-block junction with an outer predicate — suggest renaming to something like `'where query with a junction inside an embedded association block combined with another predicate'`. ########## grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java: ########## @@ -751,6 +749,33 @@ public static void populateMongoQuery(final EmbeddedQueryEncoder queryEncoder, D } } + /** + * Rewrites a query built against an embedded entity so it applies to the owning document, + * qualifying each property name with the embedded property's path. Logical operators such as + * {@code $and} and {@code $or} must stay at the current level (a key like {@code extRef1.$and} + * matches nothing), so their nested documents are rewritten recursively instead. + */ + private static void prefixEmbeddedQuery(String prefix, Document source, Document target) { Review Comment: Two notes, neither blocking: 1. `$`-keys with non-List values — `$where` (from property-to-property comparisons) and `$text` — still fall into the else branch and get mis-prefixed to e.g. `extRef1.$where`. That matches the old (broken) behavior so it's not a regression, but since this method now understands operator keys, consider either documenting the limitation here or throwing `UnsupportedOperationException` for a `$`-key that isn't a rewritable list, so it fails loudly instead of silently matching nothing. 2. Nit: raw `List`/`ArrayList` — `List<Object>` would avoid the unchecked warning, though the surrounding file is raw-typed too. ########## grails-data-hibernate7/core/src/main/groovy/grails/orm/CriteriaMethodInvoker.java: ########## @@ -193,21 +193,29 @@ protected Object tryAssociationOrJunction(String name, CriteriaMethods method, O final Metamodel metamodel = builder.getSessionFactory().getMetamodel(); final EntityType<?> entityType = metamodel.entity(builder.getTargetClass()); final Attribute<?, ?> attribute = entityType.getAttribute(name); - - if (attribute.isAssociation()) { + // The JPA metamodel does not consider an embedded component an association, but the + // GORM model does - an embedded block must build a DetachedAssociationCriteria so its + // properties resolve against the component. It needs no join: its columns live in the + // owning entity's table. + final boolean embedded = + attribute.getPersistentAttributeType() == Attribute.PersistentAttributeType.EMBEDDED; + + if (attribute.isAssociation() || embedded) { Class<?> oldTargetClass = builder.getTargetClass(); Class<?> associationClass = builder.getClassForAssociationType(attribute); builder.setTargetClass(associationClass); - JoinType joinType; - if (hasMoreThanOneArg) { - joinType = builder.convertFromInt((Integer) args[0]); - } else if (associationClass.equals(oldTargetClass)) { - joinType = JoinType.LEFT; // default to left join if joining on the same table - } else { - joinType = builder.convertFromInt(0); - } + if (!embedded) { Review Comment: Nit: an explicit join-type argument on an embedded block (e.g. `extRef1(JoinType.LEFT) { ... }`) is now silently ignored. That's the right behavior — a component can't be joined — but a short comment noting the argument is intentionally dropped (or a debug log) would help the next reader. ########## grails-test-examples/geb/src/integration-test/groovy/org/demo/spock/PerTestRecordingSpec.groovy: ########## @@ -60,62 +65,75 @@ class PerTestRecordingSpec extends ContainerGebSpec { 'build/gebContainer/recordings' ) def baseRecordingDir = new File(recordingDirectoryName) - + then: 'the base recording directory exists' baseRecordingDir.exists() - when: 'getting the most recent recording directory' - // Find the timestamped recording directory (should be the most recent one) - File recordingDir = null - def timestampedDirs = baseRecordingDir.listFiles({ File dir -> - dir.isDirectory() && dir.name ==~ /^\d{8}_\d{6}$/ - } as FileFilter) - - if (timestampedDirs) { - // Get the most recent directory - recordingDir = timestampedDirs.sort { it.name }.last() - } - - then: 'the recording directory should be found' - recordingDir != null - - when: 'getting all video recording files (mp4 or flv) from the recording directory' + when: 'collecting the recordings this spec produced during this test run' Review Comment: This flaky-test fix is sound (scoping recording-dir discovery to the current JVM's start time instead of racing on "most recent directory"), but it's unrelated to embedded where queries. Ideally it would be its own PR; at minimum please mention it in the PR description so it doesn't get lost in the merge record. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
