Extend the update()/delete() DSL with safer toggles, plus two bugs fixed along the way - #2043
Merged
ashishvijaywargiya merged 12 commits intoSep 18, 2026
Conversation
…Builder Extends the update() DSL builder to cover three previously-unsupported shapes: silent no-op on a missing record (.ifExists()), queryFirst() instead of queryOne() for legitimately-multi-match entities (.first()), and setIfEmpty=false (set(fields, false)), mirroring GenericValue.setNonPKFields(fields, setIfEmpty). All three are additive; every existing update(x).where(y).set(z) call site is unaffected.
Replaces the manual queryOne()+if/store() with the update() DSL's ifExists() toggle, which returns null instead of throwing on a missing record -- preserving this site's existing silent-no-op behavior exactly. No behavior change.
…Content() Replaces the manual queryOne()+if/store() guard on ElectronicText with the update() DSL's ifExists() toggle, which returns null instead of throwing on a missing record -- preserving this site's existing silent-no-op behavior exactly. The outer found/not-found update-vs-create branch is unchanged. No behavior change.
Replaces the manual queryOne()+if/store() guard on ElectronicText with the update() DSL's ifExists() toggle across all three title/metaKeyword/ metaDescription blocks, preserving each site's existing silent-no-op behavior exactly. The outer found/not-found update-vs-create branches are unchanged. No behavior change.
Replaces the manual queryFirst()+store() on ProductContent with the update() DSL's first() toggle, preserving the existing "take the first match" behavior exactly (this entity can legitimately have more than one matching record). No behavior change.
Previously converted in 1c118a9 and reverted in 9b0971c because EntityUpdateBuilder.set() had no setIfEmpty support and would have nulled out empty-string input fields instead of preserving their existing values. Now uses set(fields, false), wrapped in try/catch to preserve the original not-found message exactly. No behavior change.
queryOne() (the default lookup) narrows where() to PK-only fields, which is what makes it safe to pass a raw service parameters map to where(). queryFirst() (used by first()) does not do this narrowing, so a raw parameters map would incorrectly filter on non-PK fields like userLogin. Documents this caveat in both the class-level and method-level Javadoc so callers pass explicit field names to where() when using first().
first() uses queryFirst() instead of queryOne(), which does not narrow where() to PK fields internally the way queryOne()'s searchPkOnly does. A raw service parameters map (userLogin, locale, timeZone, ...) passed to .first() would build a query condition on a nonexistent column and throw. Filters where() down to the entity's own field names before querying, using setAllFields() -- the same "walk the entity's own fields, pull matching values out of the map" primitive set() already uses via setNonPKFields(), and the same one searchPkOnly itself uses internally via setPKFields() -- so first() is safe by construction instead of relying on callers reading a Javadoc caveat. Adds a regression test proving both that a raw parameters map no longer throws and that explicit field selection (today's only call site) still works.
delete('Entity').where(fields) went straight to delegator.removeByAnd()
with no filtering at all -- worse than update()'s equivalent gap, since
an empty resulting condition means DELETE FROM <table> with no WHERE
clause, removing every row. A raw service parameters map would throw
on an unrelated key like userLogin; a map with real fields that don't
happen to exist on this particular entity (a live, pre-existing bug in
WorkEffortServicesScript.groovy, which reuses one map keyed by
workEffortId against WorkEffortAssoc, whose actual PK is
workEffortIdFrom/workEffortIdTo) would silently wipe the table instead
of throwing.
Filters where() down to the entity's own field names (PK or not --
bulk-deleting by a real non-PK field, such as a foreign key shared by
several child rows, is an existing, legitimate pattern elsewhere in
this codebase and must keep working), using the same setAllFields()
primitive set() already uses via setNonPKFields(). Throws
ServiceErrorException if the filtered result is empty, so a map with
no real fields on the entity at all fails loudly instead of deleting
everything.
Regression tests cover: a raw parameters map with junk keys deletes
correctly, bulk delete by a non-PK field alone still works, and a map
with no real fields on the entity throws instead of proceeding.
Adds a chainable .orderBy(field) that passes through to the internal queryFirst()/queryOne() lookup, for .first() sites where "first" means "most recent" rather than an arbitrary match. Unblocks converting call sites that need an ordered lookup (e.g. "expire the most recent status record"), which the ifExists()/first()/setIfEmpty rollout deliberately excluded for lacking exactly this support.
It called delegator.getModelEntity(relationFieldName) instead of
delegator.getModelEntity(relationEntityName) -- e.g. getModelEntity('workEffortIdTo')
instead of getModelEntity('WorkEffortAssoc'). getModelEntity() always
returned null for the bogus name, and the following
modelEntity.getField('fromDate') threw a NullPointerException.
duplicateWorkEffort() has always failed this way whenever
duplicateWorkEffortAssocs, duplicateWorkEffortNotes,
duplicateWorkEffortContents, or duplicateWorkEffortAssignmentRates was 'Y'.
The new regression test pushed this class past codenarc's default method-count threshold. Same suppression already used elsewhere in this codebase for large, legitimately single-suite test classes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added ifExists() and first() toggles to the update() DSL, so update() can now handle a record that might not exist and an entity that can legitimately have more than one matching row, plus a setIfEmpty option that preserves existing values instead of nulling them for empty-string input fields.
Converted four call sites, updateProductCategory, CategoryContentServicesScript.updateContent, updateContentSEOForProduct, and addRejectedReasonImageManagement, to use these new toggles instead of the old manual query-then-store code.
Re-converted updatePartyTaxAuthInfo to use update() with setIfEmpty=false, which fixes the exact gap that forced this same conversion to be reverted in an earlier round.
Made first() actually safe to call with a raw service parameters map, one that still has extra keys like userLogin or locale in it.
Fixed the same missing safety in delete(), which was more serious there, since a raw or mismatched map used to go straight to a database delete with no filtering at all, meaning it could silently wipe an entire table instead of throwing an error, and this fix also caught a live example of that exact risk inside duplicateWorkEffort's cleanup code against the WorkEffortAssoc entity.
Added orderBy() support to the update() DSL, so update() can now pick the most recent matching record instead of just any matching record.
Converted ExampleServicesScript.createExampleStatus() to use the new orderBy() support, so it now expires the previous status through the update() DSL instead of a manual query and store.
Fixed a real, pre-existing bug in duplicateWorkEffortAssoc() that looked up the wrong entity name internally and crashed every single time a work effort was duplicated along with its associations, notes, contents, or assignment rates.
Testing: every conversion and every bug fix has a dedicated regression test, and the affected component suites (accounting, product, workeffort, example, service) all pass, along with checkstyleMain and codenarcMain.