Skip to content

Store the applicability per root network tag on modifications - #867

Open
flomillot wants to merge 14 commits into
mainfrom
florent/GRD-4941-modification-applicability
Open

Store the applicability per root network tag on modifications#867
flomillot wants to merge 14 commits into
mainfrom
florent/GRD-4941-modification-applicability

Conversation

@flomillot

@flomillot flomillot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Depends on gridsuite/network-modification#219.

The applicability per root network was held by study-server, attached to a (node, root network) pair, so it was lost as soon as a modification left its node — export to GridExplore then re-import, or copy/paste between studies. It is now carried by the modification itself, keyed by root network tag.

  • ModificationEntity holds applicabilityByRootNetworkTag in the modification_root_network_applicability table, copied into every DTO next to activated, so it follows the modification through every duplication path.

  • PUT /v1/network-modifications/root-network-applicability updates it for a tag, propagating to the sub modifications of a composite like the global flag does, and reading it needs no dedicated endpoint: it travels with the modifications, the metadata ones included, where it is read for the whole batch in a single query and resolved to the shared modification when the entry is a reference.

  • The application context and BuildInfos now carry the root network tag instead of a list of excluded modification uuids, and the applicator filters on isActivatedOn(tag), the single predicate shared by the five call sites.

  • A reference to a shared modification does not own its applicability: it uses the one of the modification it points to.

  • PUT /v1/network-modifications/root-network-tag renames a tag over the given modification groups and DELETE drops tags. An applicability being keyed by the tag, renaming a root network would otherwise reactivate everything the tag deactivates and deleting one would leave its entries behind. A modification a group owns is really renamed; on the shared modification a reference points to the new tag is only added, never renamed nor removed, since other studies may still name a root network of their own with the old one.

Behaviour change: isActivatedOn now covers the stashed flag too, so posting a modification with stashed=true no longer applies it to the network. It is stored stashed, as before.

Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review Change Stack

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c9185d54-f39b-4a74-9683-885b867e0a49

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e9f18877-c417-41a7-ba9f-88b0d54d985a

📥 Commits

Reviewing files that changed from the base of the PR and between 1da552b and a0f165b.

📒 Files selected for processing (4)
  • src/main/java/org/gridsuite/modification/server/NetworkModificationServerException.java
  • src/main/java/org/gridsuite/modification/server/entities/ModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/service/NetworkModificationService.java
  • src/test/java/org/gridsuite/modification/server/ModificationControllerTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/test/java/org/gridsuite/modification/server/ModificationControllerTest.java
  • src/main/java/org/gridsuite/modification/server/service/NetworkModificationService.java
  • src/main/java/org/gridsuite/modification/server/entities/ModificationEntity.java

📝 Walkthrough

Walkthrough

The change replaces explicit modification exclusions with root-network applicability. It persists applicability by root-network tag, exposes update and retrieval endpoints, propagates metadata through modification DTOs, filters nested modifications during application, and updates related tests.

Changes

Root-network applicability

Layer / File(s) Summary
Applicability data and persistence
src/main/java/org/gridsuite/modification/server/dto/*, src/main/java/org/gridsuite/modification/server/entities/*, src/main/resources/db/changelog/*
DTOs and entities now carry copied applicability maps. A dedicated Liquibase table stores modification, root-network tag, and activation values.
Recursive applicability repository APIs
src/main/java/org/gridsuite/modification/server/NetworkModificationController.java, src/main/java/org/gridsuite/modification/server/service/NetworkModificationService.java, src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java
New APIs update applicability recursively and retrieve mappings for group modifications, including nested composites and references.
Tag-based modification application
src/main/java/org/gridsuite/modification/server/modifications/NetworkModificationApplicator.java, src/main/java/org/gridsuite/modification/server/repositories/ModificationRepository.java, src/main/java/org/gridsuite/modification/server/service/NetworkModificationService.java
Build, preload, and incremental application now select modifications applicable to the group root-network tag. Explicit exclusion sets are removed.
Applicability regression coverage
src/test/java/org/gridsuite/modification/server/modifications/*, src/test/java/org/gridsuite/modification/server/service/*, src/test/java/org/gridsuite/modification/server/utils/*
Tests cover root-network filtering, updated DTO output, constructor changes, normalized applicability comparisons, and revised SQL request counts.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant NetworkModificationController
  participant NetworkModificationService
  participant NetworkModificationRepository
  participant Database
  Client->>NetworkModificationController: Update applicability
  NetworkModificationController->>NetworkModificationService: Pass UUIDs, root-network tag, and activation flag
  NetworkModificationService->>NetworkModificationRepository: Update selected and nested modifications
  NetworkModificationRepository->>Database: Store applicability mappings
  Client->>NetworkModificationController: Retrieve group applicability
  NetworkModificationController->>NetworkModificationService: Request group mappings
  NetworkModificationService->>NetworkModificationRepository: Collect nested mappings
  NetworkModificationRepository->>Database: Read applicability mappings
  Database-->>Client: Return modification applicability map
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 2.80% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The description clearly explains that applicability is stored on modifications per root network tag and identifies the related API, persistence, propagation, and application behavior changes.
Title check ✅ Passed The title clearly and concisely summarizes the main change: storing modification applicability per root network tag.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch

Warning

Your free Security trial is over. An organization admin can activate billing to continue.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/test/java/org/gridsuite/modification/server/modifications/tabularmodifications/TabularGeneratorModificationsTest.java (1)

119-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Update the historical SQL-count comments.

Several “before improvements” values do not match the changed assertions. For example, Line 119 reports 4 although the assertion changed from 5 to 6, and Line 337 reports 11 while the current assertion is also 11 and the change is from 9 to 11. Update all affected comments so the tests document the correct baselines.

Also applies to: 177-177, 189-189, 232-232, 245-245, 286-286, 299-299, 337-337, 347-347

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/test/java/org/gridsuite/modification/server/modifications/tabularmodifications/TabularGeneratorModificationsTest.java`
around lines 119 - 125, Update the historical “before improvements” SQL-count
comments in TabularGeneratorModificationsTest, including the assertions near
assertSelectCount and all indicated locations, so each comment records the
actual pre-improvement count corresponding to its current assertion change.
Preserve the assertion values and update only the affected comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@src/main/java/org/gridsuite/modification/server/modifications/NetworkModificationApplicator.java`:
- Line 205: Update the modification filter in NetworkModificationApplicator so
it requires both isApplicableOn(modificationGroupInfos.rootNetworkTag()) and
!m.getStashed(). Preserve the existing applicability behavior while excluding
stashed modifications from actual application.

In
`@src/main/java/org/gridsuite/modification/server/NetworkModificationController.java`:
- Around line 297-301: Validate the length of rootNetworkTag in
updateRootNetworkApplicability before calling
networkModificationService.updateRootNetworkApplicability, rejecting values
longer than the persisted four-character width with the controller’s established
client-error validation behavior.

---

Nitpick comments:
In
`@src/test/java/org/gridsuite/modification/server/modifications/tabularmodifications/TabularGeneratorModificationsTest.java`:
- Around line 119-125: Update the historical “before improvements” SQL-count
comments in TabularGeneratorModificationsTest, including the assertions near
assertSelectCount and all indicated locations, so each comment records the
actual pre-improvement count corresponding to its current assertion change.
Preserve the assertion values and update only the affected comments.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 18932520-f1da-42ab-97e9-62641294f223

📥 Commits

Reviewing files that changed from the base of the PR and between 9fbff91 and 24fa818.

📒 Files selected for processing (69)
  • src/main/java/org/gridsuite/modification/server/NetworkModificationController.java
  • src/main/java/org/gridsuite/modification/server/dto/BuildInfos.java
  • src/main/java/org/gridsuite/modification/server/dto/ModificationApplicationContext.java
  • src/main/java/org/gridsuite/modification/server/dto/ModificationApplicationGroup.java
  • src/main/java/org/gridsuite/modification/server/entities/CompositeModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/GroovyScriptEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/ModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/BatteryCreationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/ConverterStationCreationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/CreateCouplingDeviceEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/CreateVoltageLevelSectionEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/CreateVoltageLevelTopologyEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/GeneratorCreationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/LccCreationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/LineCreationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/LoadCreationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/ShuntCompensatorCreationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/StaticCompensatorCreationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/SubstationCreationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/TwoWindingsTransformerCreationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/VoltageLevelCreationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/creation/VscCreationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/deletion/ByFilterDeletionEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/deletion/EquipmentDeletionEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/BalancesAdjustmentEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/BatteryModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/ConverterStationModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/DeleteAttachingLineEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/DeleteVoltageLevelOnLineEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/GenerationDispatchEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/GeneratorModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/GeneratorScalingEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/LccModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/LineAttachToVoltageLevelEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/LineModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/LineSplitWithVoltageLevelEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/LinesAttachToSplitLinesEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/LoadModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/LoadScalingEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/MoveVoltageLevelFeederBaysEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/OperatingStatusModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/ShuntCompensatorModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/SubstationModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/TwoWindingsTransformerModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/VoltageInitModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/VoltageLevelModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/VoltageLevelTopologyModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/VscModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/attribute/EquipmentAttributeModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/byfilter/ByFormulaModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/equipment/modification/byfilter/ModificationByAssignmentEntity.java
  • src/main/java/org/gridsuite/modification/server/entities/tabular/TabularModificationsEntity.java
  • src/main/java/org/gridsuite/modification/server/modifications/NetworkModificationApplicator.java
  • src/main/java/org/gridsuite/modification/server/repositories/ModificationRepository.java
  • src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java
  • src/main/java/org/gridsuite/modification/server/service/NetworkModificationService.java
  • src/main/resources/db/changelog/changesets/changelog_20260806T112115Z.xml
  • src/main/resources/db/changelog/db.changelog-master.yaml
  • src/test/java/org/gridsuite/modification/server/modifications/CompositeModificationsTest.java
  • src/test/java/org/gridsuite/modification/server/modifications/EquipmentAttributeModificationTest.java
  • src/test/java/org/gridsuite/modification/server/modifications/GeneratorScalingTest.java
  • src/test/java/org/gridsuite/modification/server/modifications/LineCreationInNodeBreakerTest.java
  • src/test/java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerCreationNodeBreakerTest.java
  • src/test/java/org/gridsuite/modification/server/modifications/tabularcreations/TabularGeneratorCreationsTest.java
  • src/test/java/org/gridsuite/modification/server/modifications/tabularmodifications/TabularGeneratorModificationsTest.java
  • src/test/java/org/gridsuite/modification/server/service/BuildTest.java
  • src/test/java/org/gridsuite/modification/server/service/ModificationRepositoryTest.java
  • src/test/java/org/gridsuite/modification/server/utils/ApiUtils.java
  • src/test/java/org/gridsuite/modification/server/utils/assertions/DTOAssert.java

- require the root network tag in ModificationApplicationContext and
  ModificationApplicationGroup, their test only constructors moving to TestUtils
- make createNetworkCompositeModification transactional, copying the
  modifications reads their lazily loaded applicabilities
- drop the index on modification_id, already covered by the primary key prefix
- rename the applicability column to applicable, activated being ambiguous with
  the modification wide flag of the modification table
- rename the applicability query parameter to applicable accordingly
- add tests on the applicability update, its propagation to composite sub
  modifications and to shared modifications, and on the detached copy

Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java (3)

840-847: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Validate rootNetworkTag before persisting it.

ModificationEntity stores the map key in a column with length 4. This method writes any non-null rootNetworkTag directly at Line [847]. The controller accepts an unrestricted String. A tag longer than four characters reaches database flush and returns a server error instead of a client validation response. Enforce the tag length and format at the HTTP boundary.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java`
around lines 840 - 847, Validate rootNetworkTag at the HTTP boundary before
updateRootNetworkApplicability persists it, enforcing the existing
four-character storage limit and rejecting invalid formats with a client
validation response. Keep the repository methods updateRootNetworkApplicability
and getApplicabilityHolder focused on persistence rather than accepting
unchecked tags.

149-150: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Ensure transaction coverage for applicability reads.

replaceCompositeModification is called through NetworkModificationService.replaceCompositeModification, which is transactional. The remaining applicability-read paths require the same transaction guarantee for every caller. Add transaction boundaries where callers do not provide one.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java`
around lines 149 - 150, Review all callers of the applicability-reading logic
around createNetworkCompositeModification and replaceCompositeModification, and
add `@Transactional` boundaries to the repository methods or other entry points
that can be invoked without an existing transaction. Preserve the transaction
already supplied by NetworkModificationService.replaceCompositeModification and
ensure every path that lazily reads modification applicabilities executes within
a transaction.

524-525: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Propagate rootNetworkTag through reference resolution.

When a referenced modification is composite, loadModificationReference currently converts it with a null tag. Its inactive descendants therefore remain in referenceInfos and can be applied by NetworkModificationApplicator. Pass rootNetworkTag through the reference conversion and add a regression test for an inactive descendant in a shared composite reference.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java`
around lines 524 - 525, Update loadModificationReference and the referenced
composite conversion to accept and propagate rootNetworkTag instead of passing
null, ensuring inactive descendants are filtered from referenceInfos before
NetworkModificationApplicator applies them. Add a regression test covering an
inactive descendant in a shared composite reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In
`@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java`:
- Around line 840-847: Validate rootNetworkTag at the HTTP boundary before
updateRootNetworkApplicability persists it, enforcing the existing
four-character storage limit and rejecting invalid formats with a client
validation response. Keep the repository methods updateRootNetworkApplicability
and getApplicabilityHolder focused on persistence rather than accepting
unchecked tags.
- Around line 149-150: Review all callers of the applicability-reading logic
around createNetworkCompositeModification and replaceCompositeModification, and
add `@Transactional` boundaries to the repository methods or other entry points
that can be invoked without an existing transaction. Preserve the transaction
already supplied by NetworkModificationService.replaceCompositeModification and
ensure every path that lazily reads modification applicabilities executes within
a transaction.
- Around line 524-525: Update loadModificationReference and the referenced
composite conversion to accept and propagate rootNetworkTag instead of passing
null, ensuring inactive descendants are filtered from referenceInfos before
NetworkModificationApplicator applies them. Add a regression test covering an
inactive descendant in a shared composite reference.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 99bc57d0-5ecb-422f-8a1e-942e0b2fd803

📥 Commits

Reviewing files that changed from the base of the PR and between 8077db3 and 0645738.

📒 Files selected for processing (2)
  • src/main/java/org/gridsuite/modification/server/entities/ModificationEntity.java
  • src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java
💤 Files with no reviewable changes (1)
  • src/main/java/org/gridsuite/modification/server/entities/ModificationEntity.java

The stashed flag is now part of isActivatedOn, so the five call sites share the
same predicate instead of appending the stash check to four of them.

Behaviour change: posting a modification with stashed=true no longer applies it
to the network. The two tests relying on that now stash and restore through the
endpoint, which is what they meant to cover.

Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
…fication-applicability

# Conflicts:
#	src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java
The tag is the key of a varchar(4) column, so a longer one reached the database
and came back as a server error instead of a client one. The width becomes a
constant shared by the mapping and the check so the two can not drift apart.

Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>

@Mathieu-Deharbe Mathieu-Deharbe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.applicabilityByRootNetworkTag(copyApplicabilityByRootNetworkTag()) is missing for

public ModificationReferenceInfos toModificationInfos() {

I am not sure that this will be useful though, depending of how the applicability is handled for reference modifications.

Edit : ok nevermind it looks like it is well handled and tested in the code.

The applicability per root network tag was served by its own endpoint, which
forced an extra call on every node change and notification. It now travels with
the modifications themselves.

The metadata projections build detached entities, so they drop that lazy
collection: it is read back for the whole batch in a single query, which
resolves a reference to the shared modification it points to.

Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Warning

Your free Security trial is over. An organization admin can activate billing to continue.

The exception handling refactoring moved NetworkModificationServerException to
the error package, so the root network tag length check goes with it:
ROOT_NETWORK_TAG_TOO_LONG becomes a ModificationBusinessErrorCode mapped to a
bad request, carrying the maximum length as its business error value.

Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
Loading every activated modification of a group to then drop the ones the tag
deactivates is wasteful: ModificationEntity is a JOINED hierarchy of 36
subclasses, so each entity brings 39 left joins. The query now leaves them out,
resolving a reference to the shared modification carrying the applicability.

Only an explicit false entry excludes a modification, so a missing entry, a true
one and a null one all keep it, as the java predicate it replaces did.

Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
The query behind getActiveModifications now filters on the stash, the activation
and the root network tag, which is all isActivatedOn tests. What is applied is
decided later by the applicator anyway: this stream only picks the preloading
strategy.

Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
An applicability is keyed by the root network tag, so renaming a tag
silently reactivated everything it deactivated, and deleting a root
network left its entries behind.

Add two endpoints over the modification groups, one renaming a tag and
one dropping tags. A modification a group owns is really renamed. A
reference holds no applicability of its own: it lives on the shared
modification it points to, which other groups reference too and may
still be using the old tag for a root network of their own. There the
new tag is therefore only added, never renamed nor removed, and an
entry it already has is reused rather than overwritten.

Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
…fication-applicability

# Conflicts:
#	src/test/java/org/gridsuite/modification/server/service/ModificationRepositoryTest.java
A deactivated composite is left out as a whole, so a child it holds
never applies even when its own applicability says otherwise.

Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
PreloadingStrategy preloadingStrategy = modificationGroupsInfos.stream().map(ModificationApplicationGroup::modifications)
.flatMap(Collection::stream)
.filter(m -> m.getActivated() && !m.getStashed())
.map(ModificationInfos::getType)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preloading strategy was defined by active modifications, maybe the filter should stay and use isActivatedOn no ?


@Transactional
public void updateRootNetworkApplicability(@NonNull List<UUID> modificationUuids, @NonNull String rootNetworkTag, boolean applicable) {
modificationUuids.forEach(modificationUuid -> updateRootNetworkApplicability(getModificationEntity(modificationUuid), rootNetworkTag, applicable));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a N+1 issue here on select queries by loading modifications one by one, preloading all modifications as a first step would be judicious. Also maybe adding a test to check for query count could be great.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The insert/update count may also be affected but I'm not sure

Comment on lines +49 to +58
@Query("""
SELECT m FROM ModificationEntity m
WHERE m.container.id = :containerId AND m.stashed = false AND m.activated = true
AND (:rootNetworkTag IS NULL OR NOT EXISTS (
SELECT 1 FROM ModificationEntity holder JOIN holder.applicabilityByRootNetworkTag a
WHERE holder.id = COALESCE((SELECT r.referenceId FROM ModificationReferenceEntity r WHERE r.id = m.id), m.id)
AND KEY(a) = :rootNetworkTag AND VALUE(a) = false))
ORDER BY m.modificationsOrder
""")
List<ModificationEntity> findAllActiveModificationsByContainerId(@Param("containerId") UUID containerId,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be worth exploring whether this query is well optimized, since it's on the critical path of the node building feature.

I compared the current version against a LEFT JOIN rewrite on a container with 600 modifications, with references and exclusions. Same result (569 rows both ways), but the work done to get there is quite different:

  • resolving reference_id through a subquery does one index lookup per candidate modification — 625 lookups here, and it would be ~5000 on a group of 5000. The rewrite does it once.

SubPlan 1
-> Index Scan using "modification_referencePK" on modification_reference r
(actual time=0.000..0.000 rows=0 loops=625)

  • the holder join reads the whole modification table just to recover ids we already have. That one grows with the total table rather than the group, so it gets worse over time regardless of group size.

-> Index Only Scan using "modificationPK" on modification holder
(actual time=0.004..0.257 rows=898 loops=1)

Joining modification_reference directly makes reference_id an ordinary column and both issues go away:

SELECT m.*
FROM modification m
LEFT JOIN modification_reference r ON r.id = m.id
LEFT JOIN modification_root_network_applicability a
       ON a.modification_id = COALESCE(r.reference_id, m.id)
      AND a.root_network_tag = :tag
WHERE m.container_id = :containerId
  AND m.stashed = false
  AND m.activated = true
  AND (a.applicable IS NULL OR a.applicable = true)
ORDER BY m.modifications_order;

-> Hash Left Join (actual time=0.043..0.191 rows=597 loops=1)
Hash Cond: (m.id = r.id)
-> Index Scan using "modification_referencePK" on modification_reference r
(actual time=0.004..0.005 rows=2 loops=1)

Timings are both around 1ms at this size so they don't tell us much, the concern is the scaling.
findApplicabilitiesByIdIn has the same shape and is on the modification table loading path, so probably worth applying the same treatment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants