jamesfredley commented on code in PR #15599:
URL: https://github.com/apache/grails-core/pull/15599#discussion_r3162781575


##########
grails-doc/src/en/guide/GORM/quickStartGuide/basicCRUD.adoc:
##########
@@ -91,6 +136,128 @@ def p = Person.get(1)
 p.delete()
 ----
 
+If a delete operation fails (for example due to database constraints), an 
exception is thrown.
+
+You can handle this using a `try/catch` block:
+
+[source,groovy]
+----
+def p = Person.get(1)
+try {
+    p.delete(flush: true)
+
+} catch (Exception e) {

Review Comment:
   **Stray blank line inside `try` block.** The empty line between 
`p.delete(flush: true)` and `} catch` looks like an accidental edit and isn't 
standard Groovy formatting. Suggest removing:
   
   ```suggestion
   try {
       p.delete(flush: true)
   } catch (Exception e) {
   ```



##########
grails-doc/src/en/guide/GORM/quickStartGuide/basicCRUD.adoc:
##########
@@ -27,13 +27,45 @@ To create a domain class use Map constructor to set its 
properties and call link
 
 [source,groovy]
 ----
-def p = new Person(name: "Fred", age: 40, lastVisit: new Date())
+def p = new Person(
+    name: "Fred",
+    age: 40,
+    lastVisit: new Date(),
+)
+
 p.save()
 ----
 
 The link:{domainClassesRef}save.html[save] method will persist your class to 
the database using the underlying Hibernate ORM layer.
 
 
+=== List
+
+To retrieve multiple instances, use the `list` method:
+
+[source,groovy]
+----
+def people = Person.list()
+----
+
+This returns all `Person` records from the database.
+
+You can also pass pagination and sorting parameters:
+
+[source,groovy]
+----
+def fetchParams = [sort: 'name', order: 'asc', max: 10, offset: 0]
+def people = Person.list(fetchParams)
+----
+
+The `list` method supports:
+
+* `max` – maximum number of results
+* `offset` – starting position
+* `sort` – property to sort by
+* `order` – sort direction (`asc` or `desc`)

Review Comment:
   **Incomplete parameter list.** Accurate as far as it goes, but 
`Person.list(Map)` and `DetachedCriteria.list(Map)` (both routed through 
`DynamicFinder.populateArgumentsForCriteria`) also accept `cache`, `fetch`, 
`lock`, `readOnly`, `fetchSize`, `timeout`, `flushMode`, and `ignoreCase`. 
Consider adding a sentence like "See link:{domainClassesRef}list.html[`list`] 
for the complete list of supported parameters" so this section doesn't go stale 
as the surface evolves.



##########
grails-doc/src/en/guide/GORM/quickStartGuide/basicCRUD.adoc:
##########
@@ -91,6 +136,128 @@ def p = Person.get(1)
 p.delete()
 ----
 
+If a delete operation fails (for example due to database constraints), an 
exception is thrown.
+
+You can handle this using a `try/catch` block:
+
+[source,groovy]
+----
+def p = Person.get(1)
+try {
+    p.delete(flush: true)
+
+} catch (Exception e) {
+    println "Delete failed: ${e.message}"
+}
+----
+
+Unlike the link:{domainClassesRef}save.html[save] method, the `delete` method 
does not support a `failOnError` parameter. Instead, errors are propagated as 
exceptions.
+
+Using `flush: true` ensures the delete is executed immediately, so any errors 
are raised at that point.
+
+=== Querying
+
+To dynamically build queries based on optional parameters a common pattern is 
to use `DetachedCriteria` and progressively compose filters depending on the 
provided inputs.
+
+==== Properties
+
+Consider the following example using the `Person` domain class:
+
+[source,groovy]
+----
+import grails.gorm.DetachedCriteria
+
+private DetachedCriteria<Person> buildQuery(Map filterParams) {
+    def query = Person.where {}
+
+    if (filterParams.containsKey('id')) query = query.where {
+        id == filterParams.id
+    }
+
+    if (filterParams.containsKey('name')) query = query.where {
+        name == filterParams.name
+    }
+
+    if (filterParams.containsKey('age')) query = query.where {
+        age == filterParams.age
+    }
+
+    return query
+}
+----
+
+==== Associations
+
+You can filter by associated properties using dot notation or nested criteria. 
For example, if `Person` has a self-referencing relationship `parent`, you can 
filter by properties of the parent.
+
+Using dot notation:
+
+[source,groovy]
+----
+if (filterParams.containsKey('parent.name')) query = query.where {
+    parent.name == filterParams.'parent.name'
+}
+----
+
+Using a nested criteria block, which is useful when filtering multiple 
properties of the association:
+
+[source,groovy]
+----
+if (filterParams.containsKey('parent.name')) query = query.where {
+    parent {
+        name == filterParams.'parent.name'
+    }
+}
+----
+
+This approach allows you to:
+
+* Build queries incrementally
+* Apply only the filters that are actually provided
+* Keep query logic reusable and centralized
+
+You can then use this query in different ways.
+
+==== Find a Single Result
+
+[source,groovy]
+----
+def filterParams = [id: 1]
+def person = buildQuery(filterParams).get()
+----
+
+==== List Results
+
+[source,groovy]
+----
+def filterParams = [age: 40]
+def fetchParams = [sort: 'name', order: 'asc']
+def people = buildQuery(filterParams).list(fetchParams)
+----
+
+Filters can be combined simply by passing multiple items:
+
+[source,groovy]
+----
+def filterParams = [name: "Fred", age: 40]
+def results = buildQuery(filterParams).list()
+----
+
+==== Count Results
+
+[source,groovy]
+----
+def filterParams = [age: 40]
+def total = buildQuery(filterParams).count()
+----
+
+Each condition is applied only if the corresponding parameter exists, making 
this pattern highly flexible for search forms and APIs.
+
+==== Notes
 
+* Each call to `where {}` returns a new `DetachedCriteria`, allowing safe 
chaining.
+* This pattern avoids large, hardcoded query methods.
+* It works seamlessly with Grails’ GORM and Hibernate.

Review Comment:
   **Smart-quote apostrophe (U+2019).** `Grails'` uses a Unicode right single 
quote here. Other adoc files in `grails-doc` consistently use the ASCII 
apostrophe (`'`). Suggest:
   
   ```suggestion
   * It works seamlessly with Grails' GORM and Hibernate.
   ```



##########
grails-data-hibernate5/docs/src/docs/asciidoc/querying/whereQueries.adoc:
##########
@@ -142,6 +149,7 @@ Note that you cannot pass a closure defined as a variable 
into the `where` metho
 def callable = {
     lastName == "Simpson"
 }
+
 def query = Person.where(callable)

Review Comment:
   **Lost "won't work" signal.** This block demonstrates code that *fails* (the 
surrounding prose says "the following will produce an error"), but the code 
itself reads as a normal example. A copy/paster may not notice. Suggest 
annotating:
   
   ```suggestion
   def callable = {
       lastName == "Simpson"
   }
   
   // Fails: where() requires an inline closure for the AST transform
   def query = Person.where(callable)
   ```



##########
grails-doc/src/en/guide/GORM/quickStartGuide/basicCRUD.adoc:
##########
@@ -27,13 +27,45 @@ To create a domain class use Map constructor to set its 
properties and call link
 
 [source,groovy]
 ----
-def p = new Person(name: "Fred", age: 40, lastVisit: new Date())
+def p = new Person(
+    name: "Fred",
+    age: 40,
+    lastVisit: new Date(),
+)

Review Comment:
   **Trailing comma in named-arg constructor.** While Groovy 4 tolerates 
trailing commas in map literals, this style isn't used anywhere else in 
`grails-doc` and is non-idiomatic for Grails examples. Drop the `,` after `new 
Date()` to match conventions:
   
   ```suggestion
   def p = new Person(
       name: "Fred",
       age: 40,
       lastVisit: new Date()
   )
   ```



##########
grails-doc/src/en/guide/GORM/quickStartGuide/basicCRUD.adoc:
##########
@@ -91,6 +136,128 @@ def p = Person.get(1)
 p.delete()
 ----
 
+If a delete operation fails (for example due to database constraints), an 
exception is thrown.
+
+You can handle this using a `try/catch` block:
+
+[source,groovy]
+----
+def p = Person.get(1)
+try {
+    p.delete(flush: true)
+
+} catch (Exception e) {
+    println "Delete failed: ${e.message}"
+}
+----
+
+Unlike the link:{domainClassesRef}save.html[save] method, the `delete` method 
does not support a `failOnError` parameter. Instead, errors are propagated as 
exceptions.
+
+Using `flush: true` ensures the delete is executed immediately, so any errors 
are raised at that point.
+
+=== Querying
+
+To dynamically build queries based on optional parameters a common pattern is 
to use `DetachedCriteria` and progressively compose filters depending on the 
provided inputs.
+
+==== Properties
+
+Consider the following example using the `Person` domain class:
+
+[source,groovy]
+----
+import grails.gorm.DetachedCriteria
+
+private DetachedCriteria<Person> buildQuery(Map filterParams) {
+    def query = Person.where {}
+
+    if (filterParams.containsKey('id')) query = query.where {
+        id == filterParams.id
+    }

Review Comment:
   **Braceless `if` with multi-line indented body is misleading.** This parses 
correctly (the entire `query = query.where { ... }` is the if-body), but the 
closing `}` reads visually like the end of an `if` block, not the closure. 
Documentation examples should err toward explicit braces:
   
   ```suggestion
       if (filterParams.containsKey('id')) {
           query = query.where { id == filterParams.id }
       }
   ```
   
   Same applies to the `name` and `age` blocks below, and to the two `parent` 
examples in the Associations subsection.



##########
grails-doc/src/en/guide/GORM/quickStartGuide/basicCRUD.adoc:
##########
@@ -91,6 +136,128 @@ def p = Person.get(1)
 p.delete()
 ----
 
+If a delete operation fails (for example due to database constraints), an 
exception is thrown.
+
+You can handle this using a `try/catch` block:
+
+[source,groovy]
+----
+def p = Person.get(1)
+try {
+    p.delete(flush: true)
+
+} catch (Exception e) {
+    println "Delete failed: ${e.message}"
+}
+----
+
+Unlike the link:{domainClassesRef}save.html[save] method, the `delete` method 
does not support a `failOnError` parameter. Instead, errors are propagated as 
exceptions.
+
+Using `flush: true` ensures the delete is executed immediately, so any errors 
are raised at that point.
+
+=== Querying
+
+To dynamically build queries based on optional parameters a common pattern is 
to use `DetachedCriteria` and progressively compose filters depending on the 
provided inputs.
+
+==== Properties
+
+Consider the following example using the `Person` domain class:
+
+[source,groovy]
+----
+import grails.gorm.DetachedCriteria
+
+private DetachedCriteria<Person> buildQuery(Map filterParams) {
+    def query = Person.where {}
+
+    if (filterParams.containsKey('id')) query = query.where {
+        id == filterParams.id
+    }
+
+    if (filterParams.containsKey('name')) query = query.where {
+        name == filterParams.name
+    }
+
+    if (filterParams.containsKey('age')) query = query.where {
+        age == filterParams.age
+    }
+
+    return query
+}
+----
+
+==== Associations
+
+You can filter by associated properties using dot notation or nested criteria. 
For example, if `Person` has a self-referencing relationship `parent`, you can 
filter by properties of the parent.
+
+Using dot notation:
+
+[source,groovy]
+----
+if (filterParams.containsKey('parent.name')) query = query.where {
+    parent.name == filterParams.'parent.name'
+}
+----
+
+Using a nested criteria block, which is useful when filtering multiple 
properties of the association:
+
+[source,groovy]
+----
+if (filterParams.containsKey('parent.name')) query = query.where {
+    parent {
+        name == filterParams.'parent.name'
+    }
+}
+----
+
+This approach allows you to:
+
+* Build queries incrementally
+* Apply only the filters that are actually provided
+* Keep query logic reusable and centralized
+
+You can then use this query in different ways.
+
+==== Find a Single Result
+
+[source,groovy]
+----
+def filterParams = [id: 1]
+def person = buildQuery(filterParams).get()

Review Comment:
   Minor: `DetachedCriteria.get(Map args)` is defined at 
`grails-datamapping-core/src/main/groovy/grails/gorm/DetachedCriteria.groovy:118`
 and returns `query.singleResult()`. So this works, but consider mentioning the 
convention that `get()` (no args) on a `DetachedCriteria` returns a single 
result vs `Person.get(id)` which takes a primary key - readers transitioning 
from `Person.get(1)` may be momentarily confused by the parameterless `get()` 
here.



##########
grails-data-hibernate5/docs/src/docs/asciidoc/querying/whereQueries.adoc:
##########
@@ -262,42 +271,71 @@ Operator,Criteria Method,Description
 *<=*,sizeLe,The collection size is less than or equal to
 |===
 
-==== Query Aliases and Sorting
 
-If you define a query for an association an alias is automatically generated 
for the query. For example the following query:
+==== Sorting
+
+Sorting with `where` queries is performed by passing `sort` and (optionally) 
`order` arguments to the `list` method of a <<detachedCriteria,Detached 
Criteria>>.
+
+===== Single-field Sorting
+
+To sort by a single property:
 
 [source,groovy]
 ----
-def query = Pet.where {
-    owner.firstName == "Fred"
+def query = Person.where {
+    firstName == "Jack"
 }
+
+def results = query.list(sort: "firstName")
 ----
 
-Will generate an alias for the `owner` association such as `owner_alias_0`. 
These generated aliases are fine for most cases, but are not useful if you want 
to later sort or use a projection on the results. For example the following 
query will fail:
+By default, sorting is ascending. You can specify the direction explicitly 
using the `order` argument:
 
 [source,groovy]
 ----
-// fails because a dynamic alias is used
-Pet.where {
-    owner.firstName == "Fred"
-}.list(sort:"owner.lastName")
+def results = query.list(sort: "firstName", order: "desc")
 ----
 
-If you plan to sort the results then an explicit alias should be used and 
these can be defined by simply declaring a variable in the `where` query:
+===== Multi-field Sorting
+
+To sort by multiple fields, pass a `Map` to the `sort` argument:
+
+[source,groovy]
+----
+def results = query.list(sort: [
+    firstName: "asc",
+    birthDate: "desc"
+])
+----
+
+This sorts results first by `firstName` ascending and then by `birthDate` 
descending.
+
+===== Sorting with Associations
+
+When sorting by properties of associated entities, you must define an explicit 
alias within the `where` query:
 
 [source,groovy]
 ----
 def query = Pet.where {
-    def o1 = owner <1>
-    o1.firstName == "Fred" <2>
-}.list(sort:'o1.lastName') <3>
+    def o = owner
+    o.firstName == "Fred"
+}
+
+def results = query.list(sort: "o.lastName")
 ----
 
-<1> Define an alias called `o1`
-<2> Use the alias in the query itself
-<3> Use the alias to sort the results
+Without an explicit alias, sorting on association properties may fail due to 
dynamically generated aliases.
+
+In the example above, GORM will generate an alias for the `owner` association 
such as `owner_alias_0`. These generated aliases are fine for most cases, but 
are not useful if you want to later sort or use a projection on the results. 
For example the following query will fail:

Review Comment:
   **Logical-flow regression: backward reference to an example that doesn't 
match.** The new ordering reads:
   
   1. Show explicit-alias example (`def o = owner`) - this is the *working* case
   2. Sentence: "Without an explicit alias, sorting on association properties 
may fail..."
   3. Sentence: "**In the example above**, GORM will generate an alias for the 
`owner` association such as `owner_alias_0`..."
   4. Failing dynamic-alias example
   
   The phrase "In the example above" points back to the explicit-alias example, 
but the explanation describes the *generated*-alias case shown below it. The 
original ordering (problem -> cause -> fix) avoided this. Two options:
   
   - Restore the original order (failing example first, then explanation, then 
explicit-alias fix), or
   - Reword line 329 to "When you do not declare an explicit alias, GORM will 
generate an alias..." and move the failing example immediately after that 
sentence.
   
   Also: the original section used `<1> <2> <3>` callouts on the explicit-alias 
example ("<1> Define alias / <2> Use alias in query / <3> Use alias for sort"). 
Those were pedagogically useful; consider restoring them on lines 319-324.



##########
grails-doc/src/en/guide/GORM/quickStartGuide/basicCRUD.adoc:
##########
@@ -91,6 +136,128 @@ def p = Person.get(1)
 p.delete()
 ----
 
+If a delete operation fails (for example due to database constraints), an 
exception is thrown.
+
+You can handle this using a `try/catch` block:
+
+[source,groovy]
+----
+def p = Person.get(1)
+try {
+    p.delete(flush: true)
+
+} catch (Exception e) {
+    println "Delete failed: ${e.message}"
+}
+----
+
+Unlike the link:{domainClassesRef}save.html[save] method, the `delete` method 
does not support a `failOnError` parameter. Instead, errors are propagated as 
exceptions.
+
+Using `flush: true` ensures the delete is executed immediately, so any errors 
are raised at that point.
+
+=== Querying
+
+To dynamically build queries based on optional parameters a common pattern is 
to use `DetachedCriteria` and progressively compose filters depending on the 
provided inputs.
+
+==== Properties
+
+Consider the following example using the `Person` domain class:
+
+[source,groovy]
+----
+import grails.gorm.DetachedCriteria
+
+private DetachedCriteria<Person> buildQuery(Map filterParams) {
+    def query = Person.where {}
+
+    if (filterParams.containsKey('id')) query = query.where {
+        id == filterParams.id
+    }
+
+    if (filterParams.containsKey('name')) query = query.where {
+        name == filterParams.name
+    }
+
+    if (filterParams.containsKey('age')) query = query.where {
+        age == filterParams.age
+    }
+
+    return query
+}
+----
+
+==== Associations
+
+You can filter by associated properties using dot notation or nested criteria. 
For example, if `Person` has a self-referencing relationship `parent`, you can 
filter by properties of the parent.

Review Comment:
   **Schema for `Person` shifts mid-document.** Earlier in this file `Person` 
has `(name, age, lastVisit)`; here it gains a self-referencing `parent` 
association without being re-introduced. Suggest a short note before the 
example: "Assume `Person` also declares a self-referencing `parent` association 
(`Person parent`)." Otherwise readers may be confused why `parent` suddenly 
exists.



##########
grails-data-hibernate5/docs/src/docs/asciidoc/querying/whereQueries.adoc:
##########
@@ -262,42 +271,71 @@ Operator,Criteria Method,Description
 *<=*,sizeLe,The collection size is less than or equal to
 |===
 
-==== Query Aliases and Sorting
 
-If you define a query for an association an alias is automatically generated 
for the query. For example the following query:
+==== Sorting
+
+Sorting with `where` queries is performed by passing `sort` and (optionally) 
`order` arguments to the `list` method of a <<detachedCriteria,Detached 
Criteria>>.
+
+===== Single-field Sorting
+
+To sort by a single property:
 
 [source,groovy]
 ----
-def query = Pet.where {
-    owner.firstName == "Fred"
+def query = Person.where {
+    firstName == "Jack"
 }
+
+def results = query.list(sort: "firstName")
 ----
 
-Will generate an alias for the `owner` association such as `owner_alias_0`. 
These generated aliases are fine for most cases, but are not useful if you want 
to later sort or use a projection on the results. For example the following 
query will fail:
+By default, sorting is ascending. You can specify the direction explicitly 
using the `order` argument:
 
 [source,groovy]
 ----
-// fails because a dynamic alias is used
-Pet.where {
-    owner.firstName == "Fred"
-}.list(sort:"owner.lastName")
+def results = query.list(sort: "firstName", order: "desc")
 ----
 
-If you plan to sort the results then an explicit alias should be used and 
these can be defined by simply declaring a variable in the `where` query:
+===== Multi-field Sorting
+
+To sort by multiple fields, pass a `Map` to the `sort` argument:
+
+[source,groovy]
+----
+def results = query.list(sort: [
+    firstName: "asc",
+    birthDate: "desc"
+])

Review Comment:
   **`birthDate` is introduced without context.** Throughout this file the 
running `Person` example has only `firstName`, `lastName`, and `age`. 
`birthDate` doesn't appear anywhere else. Suggest using a property already 
established (e.g. `lastName`) so the example is self-contained:
   
   ```suggestion
   def results = query.list(sort: [
       firstName: "asc",
       lastName: "desc"
   ])
   ```
   
   Also confirms that the Map-sort feature exists in 
`DynamicFinder.applySortForMap()` (`DynamicFinder.java:550`) but had not been 
documented before this PR - good catch including it.



-- 
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]

Reply via email to