Groovy DSL Enhancement - Part 2 - #2038
Merged
ashishvijaywargiya merged 14 commits intoSep 16, 2026
Merged
ashishvijaywargiya merged 14 commits into
ashishvijaywargiya merged 14 commits into
Conversation
Converts 49 `if (cond) { return error(...) }` guard-clause sites across 8
files to require()/fail(), out of 59 `return error(...)` sites originally
scanned. Found-but-left-untouched, per file:
- AcctgAdminServices.createFuturePeriod(): 2 sites inside nested .each {}
closures -- return error() there only aborts the closure, not the service;
swapping to a throw would change that reachability without the call-site
audit this task doesn't do.
- FixedAssetServices.calculateFixedAssetDepreciation(), PaymentServices.
updatePaymentContent(), RateServices.updateRateAmount()/expireRateAmount():
4 sites shaped as "if (cond) { ...real work...; return success/store }
return error(...)" (or the equivalent if/else with substantive code in the
non-error branch) -- not the guard-clause shape this pass converts;
flattening into a leading require() would mean hoisting a branch's body out
and restructuring control flow, not a mechanical swap.
- PaymentServices.createPaymentAndApplicationForParty(),
cancelCheckRunPayments(): same non-error-branch-has-real-work shape, inside
a for loop.
- PaymentServices.createPaymentAndPaymentGroupForInvoices(): FinAccount
status checks are an if/else-if chain where every branch is a bare
return error(). Left as an if/else-if rather than split into sequential
require() calls -- that flattening is behaviorally correct here (the two
statusId values are mutually exclusive) but reads as less clear than the
original branching, so if/else-if chains are out of scope for this pass
regardless of provable mutual exclusivity.
Covered by the accounting component test suite (105/105 passing) plus
checkstyleMain/codenarcMain (clean after fixing 3 LineLength violations the
conversion introduced).
Converts 7 `if (cond) { return error(...) }` guard-clause sites across 4
files to require()/fail(), out of 16 `return error(...)` sites originally
scanned. Found-but-left-untouched, per file:
- ContentServicesScript.assocContent(): the error is an unconditional
fallback return at the end of the method, following an if-block that
returns early on success (`if (cond) { ...; return result }
return error(...)`), not a `return error(...)` inside its own guard -- not
the guard-clause shape this pass converts.
- DataServicesScript.prepareServiceContext(GenericValue, String): a
parameterized helper called as a plain Groovy method, not a top-level
service entry point invoked by the engine.
- ContentPermissionServices.viewContentPermission()/
createContentPermission()/updateContentPermission()/
checkContentOperationSecurity()/checkRoleSecurity()/
findAllContentPurposes()/findAllParentContent(): all parameterized helper
methods (5-2 args each), called as plain Groovy method calls from other
methods in this script rather than via `run service:` -- not top-level
service entries, so out of scope per the Global Constraints. (checkOwnership()
in the same file, by contrast, IS registered as its own service and
invoked via `run service: 'checkOwnership'` elsewhere in this file, so its
guard clause was converted.)
Covered by the content component test suite (14/14 passing) plus
checkstyleMain/codenarcMain (clean).
Converts 11 `if (cond) { return error(...) }` guard-clause sites across 4
files to require()/fail(), out of 14 `return error(...)` sites originally
scanned. Found-but-left-untouched:
- ProductionRunServicesScript.handleManualIssuanceOverride(Map parameters):
3 sites -- a parameterized helper called as a plain Groovy method (not via
`run service:`), not a top-level service entry point.
Two shapes worth calling out specifically since they're new to this pass:
- ProductionRunResServices.reserveWorkEffortInventory()/
reserveWorkEffortInventoryItem(): the guard has extra statements (a
GenericValue lookup, logError()) ahead of the return -- per the
extra-statement rule, the enclosing `if` was left intact and only
`return error(errMsg)` was swapped to `fail(errMsg)`, same as
ProductionRunServicesScript.issueProductionRunTask()/
issueProductionRunTaskComponent()'s two similarly-shaped sites.
- RoutingServicesScript.removeCalendar()/removeCalendarWeek(): two
independent sequential `if (cond) { return error(...) }` guards each
(not an if/else-if chain), each converted to its own require() in place --
distinct from the if/else-if flattening the accounting task's review
flagged and the Global Constraints now disallow.
Covered by the manufacturing (48/48) and marketing (1/1) component test
suites plus checkstyleMain/codenarcMain (clean).
Converts 47 `if (cond) { return error(...) }` guard-clause sites across 8
files to require()/fail(), out of 67 `return error(...)` sites originally
scanned. Found-but-left-untouched, per file:
- CheckoutMapProcs.shipToAddress(Map)/billToAddress(Map): 10 sites -- both
static methods take an explicit Map parameter and are called as plain
Groovy method calls from OrderServicesScript.groovy (a different file),
not via `run service:`; not top-level service entries. Their guard shape
is also `if (cond) { realWork } else { return error(...) }` -- the
real-work-in-the-non-error-branch shape this pass has consistently left
alone since the accounting task.
- OrderRequirementServicesScript.checkCreateStockRequirement(String
methodId): a parameterized helper whose return value is propagated
directly (implicit return, no ServiceUtil.isError check) by its two
top-level callers (checkCreateStockRequirementQoh/Atp) -- proving that
propagation safe would require auditing every call site, which this pass
doesn't do.
- OrderReturnServicesScript.informError(String label): explicitly `private`.
- OrderServicesScript: none left untouched -- all 7 sites in this file
qualified.
- QuoteServicesScript: none left untouched -- all 31 sites in this file
qualified (every method here is a top-level, no-arg service entry).
- RequirementServicesScript.deleteRequirementAndRelated()/
autoAssignRequirementToSupplier(): both are the same "if (cond) {
...real work...; return success() } return error(...)" shape as the
CheckoutMapProcs sites above.
- ShoppingListServicesScript.calculateShoppingListDeepTotalPrice(): 2 of its
4 sites are inside `.each {}` closures (lines iterating ShoppingListItem
and child ShoppingList records) -- return error() there only aborts the
closure iteration, not the service; out of scope per the Global
Constraints.
Covered by the order component test suite (99/99 passing) plus
checkstyleMain/codenarcMain (clean).
Converts 16 `if (cond) { return error(...) }` guard-clause sites across 5
files to require()/fail(), out of 23 `return error(...)` sites originally
scanned. Found-but-left-untouched, per file:
- ContactMechServicesScript.createEmailAddress()/updateEmailAddress(): the
isEmail-format check in each is `if (UtilValidate.isEmail(...)) { ...real
work...; return serviceReturn } return error(...)` -- the
real-work-in-the-non-error-branch tail shape this pass has consistently
left alone since the accounting task.
- ContactMechServicesScript.createFtpAddress(): `if (contactMechId) {
...real work... } else { return error(...) }` -- same if/else shape,
error in the else branch.
- PartyPermissionServices.accAndDecPartyInvitationPermissionCheck()/
cancelPartyInvitationPermissionCheck(): one site each is `if (cond) {
...sets hasPermission...} else { return error(...) }` -- same if/else
shape; left untouched for consistency even though the if-branch itself
doesn't return (unlike the other skipped shapes above, this one could
arguably be flattened safely, but doing so would introduce a new,
undocumented exception to the if/else policy mid-pass).
Two extra-statement guards worth noting (kept their enclosing `if`, only
`return error()` swapped to `fail()`, same as prior tasks):
- CommunicationEventServicesScript.setCommunicationEventStatus()/
setCommunicationEventRoleStatus(): logError() ahead of the error.
- PartyPermissionServices.cancelPartyInvitationPermissionCheck(): message
construction + logError() ahead of the error.
Covered by the party component test suite (96/96 passing) plus
checkstyleMain/codenarcMain (clean).
Converts 24 `return error(...)` guard-clause sites across 8 files to
require()/fail() -- all 24 sites originally scanned qualified, so nothing was
found-but-left-untouched in this part.
Per file:
- EditCategory.groovy, EditProductConfigItemContent.groovy, ImageUpload.groovy,
EditProductContent.groovy (2 sites each), SetDefaultImage.groovy (2 sites),
CostServices.groovy (1 site): all are path-traversal/upload-failure guards of
the shape `if (cond) { logError(...); return error(...) }` or with an extra
try/catch + message-building statement ahead of the error -- kept their
enclosing `if`, only `return error()` swapped to `fail()`, same as prior
tasks (the extra statements are preserved verbatim ahead of the call).
- ProductFeatureServicesScript.groovy (3 sites): plain
`if (!cond) { return error(...) }` permission/format guards in
createProductFeatureType()/createProductFeatureApplAttr(), converted to
require().
- CategoryServicesScript.groovy (10 sites across 7 service methods --
createProductCategory(), duplicateCategoryEntities(),
duplicateProductCategory(), createProductCategoryAttribute(),
updateProductCategoryAttribute(), deleteProductCategoryAttribute(),
productCategoryGenericPermission()): all bare guard clauses (permission
checks, existence checks, a checkValidDatabaseId() result check), no
accompanying else, converted to require().
Verified every converted method is a service-engine entry point (checked
against servicedef/*.xml invoke= attributes), not an internal helper --
methods like checkCategoryRelatedPermission() that are only called
internally were left alone (they use `result = error(...)` + `return result`,
not `return error(...)`, so were out of scope anyway).
Covered by the product component test suite (76/76 passing) plus
checkstyleMain/codenarcMain (clean, no line-length or other violations
introduced).
Converts 72 `return error(...)` guard-clause sites across 8 files to
require()/fail(). 9 sites were found but left untouched, called out below with
the reason.
Per file:
- ImageManagementServicesScript.groovy (19 sites): bare guards in
uploadProductImages()/removeProductContentAndImageFile()/
removeProductContentForImageManagement()/createImageContentApproval()/
resizeImages(), including guards nested inside for-loops (not closures, so
in scope) and one guard-with-extra-statements (kept its if, swapped
return error() to fail()). Left untouched: removeImageBySize()'s
return error() inside a contentAssocs.each {} closure -- scoped to the
closure only.
- InventoryServicesScript.groovy (7 sites): bare guards in
facilityGenericPermission()/checkProductFacilityRelatedPermission()/
createInventoryItem(). Left untouched: checkFacilityRelatedPermission() --
an internal helper not registered as its own service, only called via plain
method calls; createInventoryItem()'s lot-id if/else-if chain -- excluded
per the if/else-if rule.
- PriceServicesScript.groovy (7 sites): permission guards in
createProductPrice()/updateProductPrice()/deleteProductPrice()/
createProductPriceCond()/updateProductPriceCond(). Left untouched:
inlineHandlePriceWithTaxIncluded()'s tax-percentage guard -- an internal
helper not registered as its own service.
- ProductServicesScript.groovy (7 sites): guards in createProduct()/
duplicateProduct()/productGenericPermission()/productPriceGenericPermission(),
plus a guard-with-extra-statements in setProductReviewStatus(). Left
untouched: checkProductRelatedPermission(String, String)'s permission guard
-- an internal helper (only checkProductRelatedPermissionService() wraps it
in servicedef).
- PromoServicesScript.groovy (1 site): updateProductPromoCond()'s unconditional
trailing return error() (already the terminal statement, no guarding if to
negate) swapped directly to fail().
- ProductStoreServices.groovy (13 sites): guards in createProductStore()/
updateProductStore()/reserveStoreInventory()/isStoreInventoryAvailable(),
plus a guard-with-extra-statements in productStoreGenericPermission(). One
of the 13 was return error(...) with a double space, missed by a literal
return error( grep but caught by a follow-up scan and converted like its
neighbors. Left untouched: checkProductStoreRelatedPermission(Map)'s
permission guard -- an internal helper not registered as its own service.
- ShipmentReceiptServices.groovy (2 sites): both in receiveInventoryProduct(),
one a bare compound guard, one a guard-with-extra-statements.
- ShipmentServices.groovy (16 sites): bare and guard-with-extra-statements
sites across updateShipment()/deleteShipmentPackage()/quickShipEntireOrder()/
quickDropShipOrder()/createOrderShipmentPlan()/quickShipOrderByItem()/
addOrderShipmentToShipment(). Left untouched: createShipmentForReturn()'s
if/else-if/else chain -- excluded per the if/else-if rule;
getOrderItemShipGroupLists()'s guard -- an internal helper not registered as
its own service.
Every converted (or containing) method was verified against servicedef/*.xml
invoke= attributes as a direct service-engine entry point; every excluded
helper was excluded specifically because it is not registered there under its
own name.
Covered by the product component test suite (76/76 passing) plus
checkstyleMain/codenarcMain (clean, no violations introduced).
…/common Converts 27 `return error(...)` guard-clause sites across 5 files to require()/fail(). 2 sites were found but left untouched, called out below with the reason. Per file: - WorkEffortServicesScript.groovy (18 sites): bare guards in checkAndCreateWorkEffort()/checkAndUpdateWorkEffort()/createWorkEffort()/ updateWorkEffort()/deleteWorkEffort()/copyWorkEffort()/duplicateWorkEffort()/ assocAcceptedCustRequestToWorkEffort()/assignPartyToWorkEffort()/ createWorkEffortContactMech()/updateWorkEffortContactMech(), including guards nested inside regular if blocks (not closures, so in scope), plus a guard-with-extra-statements in createWorkEffortAndPartyAssign() (kept its if, swapped return error() to fail()). - CommonServicesScript.groovy (3 sites): bare guards in convertUom()/ convertUomCustom()/getVisualThemeResources(). - PortalPageServices.groovy (2 sites): bare guards in getPortletAttributes()/ updatePortletSeqDragDrop(). Left untouched: checkOwnerShip()'s two return error() sites -- a private helper called from several top-level methods; proving every caller safely propagates its result requires auditing every call site, which this pass does not do. - EmailServicesScript.groovy (1 site): a guard-with-extra-statements in sendMailFromTemplateSetting() (kept its if/logError, swapped return error() to fail()). - CommonPermissionServices.groovy (3 sites): bare guards in genericBasePermissionCheck()/getAllCrudPermissions(). Every converted (or containing) method was verified against servicedef/*.xml invoke= attributes as a direct service-engine entry point; checkOwnerShip() was excluded specifically because it is a private helper, not a registered service. Covered by the workeffort (31/31) and common (5/5) component test suites, plus checkstyleMain/codenarcMain (clean, no violations introduced).
…e inverted and incomplete
Both methods errored when the user HAD the WORKEFFORTMGR_DELETE permission
and lacked a personal WorkEffortPartyAssignment for the target work effort,
the opposite of the intended check (comment: must be associated OR have the
corresponding permission). This let a user with neither an assignment nor
the permission through, while blocking one who had the permission.
Also switched from hasPermission('WORKEFFORTMGR_DELETE', ...) to
hasEntityPermission('WORKEFFORTMGR', '_DELETE', ...) so a WORKEFFORTMGR_ADMIN
grant satisfies the check too, matching the existing WorkEffortDeletePermissionError
label text ("... WORKEFFORTMGR_DELETE or WORKEFFORTMGR_ADMIN permission") and
the SUPER security group's actual grant (WORKEFFORTMGR_ADMIN, not _DELETE) in
applications/datamodel/data/seed/WorkEffortSeedData.xml.
Found while working on the require()/fail() DSL rollout (unrelated task);
fixed as its own commit since it is a real logic change, not a mechanical
conversion. workeffort test suite: 31/31 passing (was 30/31 with the plain
negation fix alone, since the system test user only holds WORKEFFORTMGR_ADMIN
via SUPER). checkstyleMain/codenarcMain clean.
Converts 5 sync sends-a-notification-email call sites, across 3 files, to
runAsync service:/with:, out of the ~10 Email/Notification-named service
calls audited across the 5 files listed for this task. Every call site was
read in its full calling method, not matched by name alone.
Converted (both Row 4b conditions hold -- no result field read afterward,
and nothing about correctness depends on completion before the method
continues):
- OrderDeliveryServices.sendOrderDeliveryScheduleNotification(): the
sendGenericNotificationEmail call is the last meaningful action before
return success(); its result was never assigned to a variable.
- CommunicationEventServicesScript.sendContactUsEmailToCompany(): the
sendMailFromScreen call is the last statement in the method (inside the
bodyScreenLocation-present branch), unassigned result.
- PartyServicesScript.sendCreatePartyEmailNotification(),
sendUpdatePersonalInfoEmailNotification(),
sendAccountActivatedEmailNotification(): all three are dedicated
"send a notification email" methods whose only real work, after building
the email params, is the sendMailFromScreen call; each is the last
statement in the method and its result is never assigned or checked.
sendMailFromScreen's own service name doesn't literally contain "Email"/
"Notif" (unlike sendGenericNotificationEmail or findPartyFromEmailAddress),
but it's the same fire-and-forget email-sending service used elsewhere in
this plan's precedent (accounting/InvoiceServicesScript.sendInvoicePerEmail
already calls it via runAsync), and four of these five sites are calling
methods explicitly named/documented as "...Notification" sends -- judged
in scope on that basis rather than a literal substring match.
Left synchronous (found, audited, and excluded -- not silently skipped):
- CheckoutServices.createUpdateCustomerAndShippingAddress():
createUpdatePartyEmailAddress's result (serviceResultCUPEM) is read
immediately afterward for parameters.emailContactMechId and
result.emailContactMechId, which also drives a subsequent
shoppingCart.addContactMechId() call -- condition (a) fails.
- CommunicationEventServicesScript.createCommunicationEvent() (x1),
.updateCommunicationEvent() (x2), .sendContactUsEmailToCompany() (x1):
all four getPartyEmail calls have their result's contactMechId/
emailAddress field read and used in the same method immediately after
the call -- condition (a) fails.
- CommunicationEventServicesScript.sendEmailDated(): the raw
dispatcher.runSync('sendCommEventAsEmail', serviceContext, 7200, true)
passes an explicit transactionTimeout and requireNewTransaction=true --
an inline comment states this bypasses the run service:/with: sugar
specifically for "the new transaction need". runAsyncService(name,
ctxMap) has no equivalent parameters, so converting would silently drop
that transactional isolation between each loop iteration's email send,
which is more than a sync->async change -- left as-is.
- PartyPermissionServices.accAndDecPartyInvitationPermissionCheck() and
.cancelPartyInvitationPermissionCheck(): both findPartyFromEmailAddress
calls have their result's partyId read immediately afterward to decide
hasPermission -- condition (a) fails.
- PartyServicesScript.quickCreateCustomer(): createPartyEmailAddress's
result is immediately checked with ServiceUtil.isError() and returned
on failure -- condition (b) fails.
Covered by the order and party component test suites (99 + 96 tests, all
passing) plus checkstyleMain/codenarcMain.
Converts 1 sync sends-a-notification-email call site, out of the 4
Email/Notification-named service calls audited across the 3 files listed for
this task. Every call site was read in its full calling method, not matched
by name alone.
Converted (both Row 4b conditions hold -- no result field read afterward,
and nothing about correctness depends on completion before the method
continues):
- ShipmentServices.sendShipmentScheduledNotification(): the
sendGenericNotificationEmail call is the last meaningful action before
return success() -- the else branch of its guarding if/else only logs, and
execution falls through to return success() either way; its result was
never assigned to a variable.
Left synchronous (found, audited, and excluded -- not silently skipped):
- ProductContentServicesScript.createEmailContentForProduct(): the
createEmailContent call's result (serviceResult) has its contentId field
read immediately afterward and assigned into createProductContent.contentId,
which is then used by the following createProductContent call -- condition
(a) fails. (This service creates a Content record of an email-template
type; it doesn't send anything, but its name literally contains "Email" so
it was audited on that basis.)
- EmailServicesScript.sendMailFromTemplateSetting(): both calls in this
method fail the test --
- getPartyEmail's result has its emailAddress field read immediately
afterward into parameters.sendTo, which is then checked and used to
decide whether to proceed -- condition (a) fails.
- sendMailFromScreen's result has messageWrapper, body, and
communicationEventId fields read afterward into the method's own result
map, and is also checked with ServiceUtil.isSuccess() with an early
return on failure -- both conditions (a) and (b) fail. This is the
self-referential case flagged going into this task (one email/notification
service calling another internally); each call was read on its own
merits, not assumed in-scope because the file is about email.
Covered by the product and common component test suites (76 + 5 tests, all
passing) plus checkstyleMain/codenarcMain.
…ad-validation scripts EditCategory.groovy, EditProductConfigItemContent.groovy, EditProductContent.groovy, ImageUpload.groovy, and SetDefaultImage.groovy are invoked as widget <script> actions or mini-lang <script location=.../> events, not as declared services. Unlike GroovyEngine.runSync, those invocation paths (AbstractModelAction.ScriptAction, CallScript.exec via ScriptUtil.executeScript) do not catch ServiceErrorException, so a routine upload/path-traversal validation failure was surfacing as an uncaught exception instead of the expected inline error message. Reverted these 10 guard-clause sites back to return error(...).
sendGenericNotificationEmail (OrderDeliveryServices, ShipmentServices) and sendMailFromScreen (CommunicationEventServicesScript, PartyServicesScript x3) were switched from run service: to runAsync service:, so a mail-config or template failure no longer propagated back to the calling service -- callers now returned success even though the notification email silently failed to send. Reverted these 5 sites back to run service:.
…s positive require() checks PaymentServices.groovy's createPayment()/updatePayment() permission check and LeadServices.groovy's createLead() name/group guard each had a nested double-negation (require(!(!a && (!b && c && d)), ...)) that reproduced the exact expression shape already responsible for one real inverted-permission-check bug on this branch (deleteWorkEffort/duplicateWorkEffort, commit 515fa7e). Rewrote both via De Morgan's laws into a direct positive require() with zero negations, verified logically equivalent to the original by exhaustive truth-table check across all boolean inputs. No behavior change.
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.
Applied the require()/fail() control-flow sugar across accounting, content, manufacturing, marketing, order, party, product, workeffort, and framework/common, converting return error(...) guard clauses to throw-based validation so failures propagate correctly out of nested closures and helper methods instead of silently falling through.
Applied the runAsyncService()/runAsync DSL sugar to notification-email sends in product, framework/common, order, and party, converting manual dispatcher.runAsync() calls and select synchronous run service: calls to the equivalent one-line async form.
Found and fixed a real pre-existing bug uncovered during the rollout: deleteWorkEffort and duplicateWorkEffort had permission checks that were inverted and incomplete, allowing/denying access incorrectly.
Full branch review found and fixed a regression where fail()/require() was left uncaught in 5 files invoked as widget or mini-lang scripts rather than declared services (EditCategory, EditProductContent, EditProductConfigItemContent, ImageUpload, SetDefaultImage), reverting those 10 guard sites back to the request-aware return error(...) pattern.
Full branch review found and fixed a regression where 5 notification-email call sites had been switched to runAsync service:, silently dropping error propagation to the caller; reverted all 5 back to synchronous run service:.
Rewrote two compound double-negation guards, PaymentServices.groovy's createPayment()/updatePayment() permission check and LeadServices.groovy's createLead() name/group check, into fully positive require() conditions via De Morgan's laws, verified logically equivalent to the originals by exhaustive truth-table check.