Conversation
…#19) * Update README for staged submodel composition and remove extra spec doc * Fix staged fragment idempotency and preserve internal submodel compatibility
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 29 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds staged submodel composition: public AddSubmodel(Submodel) to register validated submodels and AddSubmodelFragment(submodelId, ...) to queue fragment contributions. Build() now detects duplicate submodel IDs, applies queued fragments in registration order (with rollback semantics on failure), and validates shell→submodel references at build time. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Builder as AasBuilder
participant Fragment as SubmodelFragmentBuilder
participant Env as Environment
Client->>Builder: AddSubmodel(submodel)
Builder->>Builder: Validate submodel.Id & IdShort
Builder->>Builder: Register submodel
Client->>Builder: AddSubmodelFragment(submodelId, configure)
Builder->>Builder: Validate submodelId & configure
Builder->>Builder: Enqueue fragment action
Client->>Builder: Build()
Builder->>Builder: Detect duplicate submodel IDs
Builder->>Builder: For each queued fragment: locate base Submodel
alt base found
Builder->>Fragment: Initialize with target Submodel
Fragment->>Fragment: Apply fragment (AddProperty/AddElement...)
Builder->>Builder: Commit fragment changes
else base missing
Builder->>Builder: Throw InvalidOperationException ("No base submodel")
Builder->>Builder: Rollback applied fragments
end
Builder->>Builder: Validate shell→submodel references
Builder->>Env: Return built environment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cb0629112
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+157
to
+160
| applyFragment(submodel); | ||
| } | ||
|
|
||
| _submodelFragments.Clear(); |
There was a problem hiding this comment.
Build() mutates submodels by calling applyFragment(submodel) before it knows the whole fragment batch is valid, and _submodelFragments is only cleared after the loop. If a later fragment fails (e.g., missing target submodel or a thrown configure callback), earlier fragments remain applied while the fragment queue is retained, so retrying Build() after fixing the issue re-applies earlier fragments and duplicates elements. This creates non-atomic failure behavior and can silently corrupt composed submodels during recovery flows.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
FluentAAS/FluentAAS.Builder/AasBuilder.cs (1)
149-189:⚠️ Potential issue | 🟠 Major
Build()is mutating shared submodel instances in place.Lines 149-189 apply fragments directly to
_submodelsand then return_submodels.ToList(), which is only a shallow copy. That means a laterAddSubmodelFragment(...); Build();can retroactively change submodels that were already exposed through an earlierEnvironment, and any exception during fragment application or later validation leaves partially-applied state behind. Please build against a per-call clone/snapshot and only commit it after the full build succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@FluentAAS/FluentAAS.Builder/AasBuilder.cs` around lines 149 - 189, Build() currently mutates the shared _submodels in place when applying _submodelFragments and then returns a shallow copy, so partially-applied fragments may leak or later calls can retroactively change previously returned Environment instances; to fix, create per-call snapshots: clone _submodels (and _shells) into local lists at the start of Build() (use an existing Submodel.Clone()/copy constructor or implement a deep copy for Submodel), apply all applyFragment actions to the cloned submodels collection, run the validation against the cloned lists (use the cloned knownSubmodelIds and iterate cloned shells), and only if all fragment-application and validation succeed return a new Environment built from the cloned lists (do not mutate the original _submodels/_shells or clear _submodelFragments until after success).
🧹 Nitpick comments (1)
FluentAAS/FluentAAS.Builder.Tests/AasBuilderTests.cs (1)
193-267: Add coverage for shell-to-submodel reference validation.The new
Build()branch inFluentAAS/FluentAAS.Builder/AasBuilder.csLines 167-180 is part of this PR’s contract, but these additions only lock down duplicate IDs and fragment targets. Please add one passing case and one failing case for shell submodel references so this validation cannot regress silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@FluentAAS/FluentAAS.Builder.Tests/AasBuilderTests.cs` around lines 193 - 267, Add two tests in AasBuilderTests that exercise the new shell→submodel reference validation: (1) a passing test that calls AddSubmodel(<id>) then adds a Shell with a submodel reference to that same id (use AddShell or the builder method that attaches submodel refs) and then calls Build() expecting success and the shell to contain the reference; (2) a failing test that adds a Shell which references a non-existent submodel id and then asserts Build() throws InvalidOperationException with the validation message (invoke Build() the same way as other tests). Use the existing CreateSut(), AddSubmodel(...) and Build() helpers to locate where to add these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 211: The README claim that AddExistingSubmodel(...) delegates to
AddSubmodel(...) is inaccurate because AddExistingSubmodel currently calls
AddSubmodelInternal() directly in AasBuilder (see AddExistingSubmodel and
AddSubmodelInternal); either update AddExistingSubmodel to call the public
AddSubmodel(...) so it inherits the same validation/behavior, or change the
README line to state that AddExistingSubmodel calls AddSubmodelInternal and does
not perform AddSubmodel's public validation — pick one and make the code or
documentation consistent, referencing the AddExistingSubmodel, AddSubmodel, and
AddSubmodelInternal symbols in AasBuilder.cs.
---
Outside diff comments:
In `@FluentAAS/FluentAAS.Builder/AasBuilder.cs`:
- Around line 149-189: Build() currently mutates the shared _submodels in place
when applying _submodelFragments and then returns a shallow copy, so
partially-applied fragments may leak or later calls can retroactively change
previously returned Environment instances; to fix, create per-call snapshots:
clone _submodels (and _shells) into local lists at the start of Build() (use an
existing Submodel.Clone()/copy constructor or implement a deep copy for
Submodel), apply all applyFragment actions to the cloned submodels collection,
run the validation against the cloned lists (use the cloned knownSubmodelIds and
iterate cloned shells), and only if all fragment-application and validation
succeed return a new Environment built from the cloned lists (do not mutate the
original _submodels/_shells or clear _submodelFragments until after success).
---
Nitpick comments:
In `@FluentAAS/FluentAAS.Builder.Tests/AasBuilderTests.cs`:
- Around line 193-267: Add two tests in AasBuilderTests that exercise the new
shell→submodel reference validation: (1) a passing test that calls
AddSubmodel(<id>) then adds a Shell with a submodel reference to that same id
(use AddShell or the builder method that attaches submodel refs) and then calls
Build() expecting success and the shell to contain the reference; (2) a failing
test that adds a Shell which references a non-existent submodel id and then
asserts Build() throws InvalidOperationException with the validation message
(invoke Build() the same way as other tests). Use the existing CreateSut(),
AddSubmodel(...) and Build() helpers to locate where to add these tests.
🪄 Autofix (Beta)
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
Run ID: 12064ba2-38cc-44d6-bdd1-31274dee580f
📒 Files selected for processing (5)
FluentAAS/FluentAAS.Builder.Tests/AasBuilderTests.csFluentAAS/FluentAAS.Builder/AasBuilder.csFluentAAS/FluentAAS.Builder/IAasBuilder.csFluentAAS/FluentAAS.Builder/SubModel/SubmodelFragmentBuilder.csREADME.md
Contributor
|
Note Docstrings generation - SUCCESS |
coderabbitai Bot
added a commit
that referenced
this pull request
Mar 27, 2026
…ddsubmodel-at-any-stage-of-the-build-process` Docstrings generation was requested by @Freezor. * #20 (comment) The following files were modified: * `FluentAAS/FluentAAS.Builder/AasBuilder.cs` * `FluentAAS/FluentAAS.Builder/IAasBuilder.cs` * `FluentAAS/FluentAAS.Builder/SubModel/SubmodelFragmentBuilder.cs`
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
…ddsubmodel-at-any-stage-of-the-build-process` (#21) Docstrings generation was requested by @Freezor. * #20 (comment) The following files were modified: * `FluentAAS/FluentAAS.Builder/AasBuilder.cs` * `FluentAAS/FluentAAS.Builder/IAasBuilder.cs` * `FluentAAS/FluentAAS.Builder/SubModel/SubmodelFragmentBuilder.cs` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
|
✅ Unit tests committed locally. Commit: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
FluentAAS/FluentAAS.Builder/IAasBuilder.cs (1)
98-108:⚠️ Potential issue | 🔴 CriticalFix malformed XML documentation for
AddSubmodelInternal.Duplicate documentation blocks.
🐛 Proposed fix
/// <summary> /// Adds a fully constructed <see cref="Submodel"/> to the environment. /// Intended for internal use by shell or submodel builders. /// </summary> /// <param name="submodel">The submodel instance to add.</param> - /// <summary> -/// Adds a fully constructed Submodel to the builder's environment. -/// </summary> -/// <param name="submodel">The fully constructed Submodel to add to the environment.</param> -/// <exception cref="ArgumentNullException">Thrown when <paramref name="submodel"/> is null.</exception> + /// <exception cref="ArgumentNullException">Thrown when <paramref name="submodel"/> is null.</exception> void AddSubmodelInternal(Submodel submodel);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@FluentAAS/FluentAAS.Builder/IAasBuilder.cs` around lines 98 - 108, Fix the malformed/duplicated XML doc for the AddSubmodelInternal method: remove the duplicated summary/param block and leave a single well-formed XML comment that documents the method, its parameter Submodel submodel, and the ArgumentNullException thrown when submodel is null; update the XML to use one <summary> and one <param> (and one <exception>) pair above the void AddSubmodelInternal(Submodel submodel) declaration so the compiler XML parser sees a single coherent doc block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@FluentAAS/FluentAAS.Builder/AasBuilder.cs`:
- Around line 154-168: The XML doc for the Build method is malformed: there is
an open <returns> tag that isn't closed before a new <summary> starts and there
are duplicated summary/returns blocks; fix by collapsing to a single well-formed
XML comment block for the Build() method (remove the duplicated <summary> and
ensure the <returns> element is properly closed), keeping the exception tags
as-is and preserving the method signature public IEnvironment Build().
- Around line 107-114: The XML documentation for AddExistingSubmodel contains
duplicated/malformed blocks; fix it by consolidating into a single well-formed
XML doc summary/param/returns/exception set for the method AddExistingSubmodel
in class AasBuilder (remove the duplicate documentation block, ensure <summary>,
<param name="submodel">, <returns>, and <exception> tags are present and
correctly closed, and that <see cref="Submodel"/> and <c>Id</c> usages are
correctly formatted).
- Around line 120-136: Remove the duplicate/malformed XML documentation above
the AddSubmodelFragment method: keep a single well-formed /// <summary> block
describing that it stages a submodel fragment for Build, one set of /// <param
name="submodelId"> and /// <param name="configure"> entries, and the correct ///
<returns> and /// <exception> tags (ArgumentException for null/empty/whitespace
submodelId and ArgumentNullException for null configure). Ensure the remaining
XML references the method signature (AddSubmodelFragment), the
SubmodelFragmentBuilder type, and IAasBuilder return type, and that there are no
repeated or overlapping docblocks.
- Around line 44-54: The XML doc for AddShell is malformed because an
<exception> tag is opened before the <summary> and not closed; fix the AddShell
documentation by making the XML well-formed: place the <summary> block first,
then a single properly closed <exception
cref="ArgumentException">...</exception> (or remove the duplicate opening tag),
ensure the <param> and <returns> tags remain intact and that AddShell's
documentation contains only one <exception> element describing the id/idShort
validation.
- Around line 86-95: The XML doc for AddSubmodel has an unclosed <exception> tag
before the <summary>, which breaks documentation parsing; fix by restructuring
the XML so the <summary> comes first followed by <param>, <returns>, and then
properly formed <exception> elements (or close the initial <exception/> tag)
referencing the AddSubmodel method and its parameter submodel and the
ArgumentNullException/ArgumentException cases; ensure all tags are well-formed
and correctly nested for the AddSubmodel documentation.
- Around line 246-250: The XML doc for AddSubmodelInternal contains
duplicate/malformed blocks; open the AddSubmodelInternal method and remove the
redundant XML documentation so there is a single well-formed <summary>, a single
<param name="submodel"> describing non-null and non-empty Id, and a single
<exception cref="ArgumentNullException"> entry; ensure tags are syntactically
correct (properly closed) and that the summary/param/exception accurately
describe AddSubmodelInternal's behavior.
In `@FluentAAS/FluentAAS.Builder/IAasBuilder.cs`:
- Around line 63-79: The XML docs for AddSubmodelFragment are duplicated and
malformed; consolidate them into a single well-formed documentation block above
the IAasBuilder.AddSubmodelFragment declaration that includes a summary, param
tags for submodelId and configure (referencing SubmodelFragmentBuilder), a
returns tag, and the two exception tags (ArgumentException for invalid
submodelId and ArgumentNullException for null configure); remove the
redundant/extra documentation block so only one correct XML comment remains
directly above the AddSubmodelFragment method signature.
- Around line 50-61: The XML docs for IAasBuilder.AddExistingSubmodel are
malformed due to duplicated/misaligned <summary> blocks; fix by consolidating
into a single well-formed XML doc comment for the AddExistingSubmodel method
that includes one <summary> describing the method, a single <param
name="submodel"> tag, a <returns> tag returning IAasBuilder, and the <exception
cref="ArgumentNullException"> tag; update the documentation immediately above
the IAasBuilder AddExistingSubmodel signature and remove the duplicated/extra
summary block so the comments are valid XML and correspond to the
AddExistingSubmodel(Submodel) member.
- Around line 39-48: The XML doc for IAasBuilder.AddSubmodel is malformed
because an <exception> tag is left unclosed and appears before the <summary>;
open/close XML tags must be well-formed and in logical order. Edit the XML
comment for the AddSubmodel method (IAasBuilder.AddSubmodel) to ensure a single
well-formed <summary> block followed by properly closed <param> and <returns>
blocks and then properly formed <exception> tags (e.g., <exception
cref="ArgumentNullException">...</exception> and <exception
cref="ArgumentException">Thrown when required submodel fields (for example, <see
cref="IReferable.IdShort"/>) are null, empty, or otherwise
invalid.</exception>), removing the stray/unclosed tag so the comment parses
correctly.
- Around line 20-30: The XML doc for IAasBuilder.AddShell is malformed because
an <exception> tag isn't closed before the <summary>, causing CS1570; open
IAasBuilder.cs and fix the AddShell documentation by removing the stray/unclosed
<exception> block and leaving a single well-formed <exception
cref="ArgumentException"> element (closed and containing the intended text about
id/idShort being null/empty/whitespace), ensure the <summary>, <param
name="id">, <param name="idShort">, <param name="kind">, <returns>, and the
correctly closed <exception> entries are present and properly ordered for the
AddShell method signature.
In `@FluentAAS/FluentAAS.Builder/SubModel/SubmodelFragmentBuilder.cs`:
- Around line 93-104: The XML docs for AddElement in SubmodelFragmentBuilder
contain duplicate <summary> blocks and malformed XML; edit the doc comment for
the AddElement(ISubmodelElement element) method to have a single well-formed
<summary> (one opening and one closing tag) describing the method, keep the
existing <param name="element">, <returns>, and <exception
cref="ArgumentNullException"> tags, remove the duplicated summary text, and
ensure all XML tags are properly closed so the documentation compiles cleanly.
- Around line 62-77: The XML doc for AddMultiLanguageProperty contains duplicate
<summary> blocks and an unclosed <exception> tag; fix it by consolidating the
descriptions into a single well-formed XML doc block for the
AddMultiLanguageProperty method (referencing parameters idShort and configure
and return type SubmodelFragmentBuilder), ensure each <exception>
(ArgumentException, ArgumentNullException) is correctly opened and closed, and
remove the redundant/duplicate summary block so the documentation is valid and
consistent with the method signature and LangStringSetBuilder usage.
- Around line 22-40: The XML doc comments for AddProperty,
AddMultiLanguageProperty, and AddElement are malformed due to
duplicated/unfinished tags (an <exception> left unclosed before a new
<summary>), causing CS1570; open the file and for each affected method
(AddProperty, AddMultiLanguageProperty, AddElement) either remove the duplicated
appended docblock or properly close the original XML tags so each XML section is
well-formed (each <summary> and <exception> has a matching closing tag),
ensuring the resulting comment for each method contains only one coherent
<summary>, parameter descriptions, and properly closed <exception> elements.
---
Outside diff comments:
In `@FluentAAS/FluentAAS.Builder/IAasBuilder.cs`:
- Around line 98-108: Fix the malformed/duplicated XML doc for the
AddSubmodelInternal method: remove the duplicated summary/param block and leave
a single well-formed XML comment that documents the method, its parameter
Submodel submodel, and the ArgumentNullException thrown when submodel is null;
update the XML to use one <summary> and one <param> (and one <exception>) pair
above the void AddSubmodelInternal(Submodel submodel) declaration so the
compiler XML parser sees a single coherent doc block.
🪄 Autofix (Beta)
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
Run ID: 91f3efc3-2617-4fd5-b554-ff31cb56aa8e
📒 Files selected for processing (3)
FluentAAS/FluentAAS.Builder/AasBuilder.csFluentAAS/FluentAAS.Builder/IAasBuilder.csFluentAAS/FluentAAS.Builder/SubModel/SubmodelFragmentBuilder.cs
Comment on lines
+120
to
+136
| /// <summary> | ||
| /// Adds a staged fragment to an already registered submodel, identified by submodel id. | ||
| /// Fragments are applied in registration order during <see cref="Build"/>. | ||
| /// </summary> | ||
| /// <param name="submodelId">The identifier of the target submodel.</param> | ||
| /// <param name="configure">Callback that contributes submodel elements.</param> | ||
| /// <returns>The current <see cref="IAasBuilder"/> instance for fluent chaining.</returns> | ||
| /// <exception cref="ArgumentException">Thrown when <paramref name="submodelId"/> is null, empty, or whitespace.</exception> | ||
| /// <summary> | ||
| /// Stages a submodel fragment configuration to be applied to the submodel with the specified id during Build. | ||
| /// </summary> | ||
| /// <param name="submodelId">Identifier of the target submodel; must not be null, empty, or whitespace.</param> | ||
| /// <param name="configure">Callback that receives a <see cref="SubmodelFragmentBuilder"/> to configure the fragment.</param> | ||
| /// <returns>The current <see cref="IAasBuilder"/> instance for fluent chaining.</returns> | ||
| /// <exception cref="ArgumentException">Thrown when <paramref name="submodelId"/> is null, empty, or whitespace.</exception> | ||
| /// <exception cref="ArgumentNullException">Thrown when <paramref name="configure"/> is null.</exception> | ||
| public IAasBuilder AddSubmodelFragment(string submodelId, Action<SubmodelFragmentBuilder> configure) |
Contributor
There was a problem hiding this comment.
Fix malformed XML documentation for AddSubmodelFragment.
Duplicate documentation blocks.
🐛 Proposed fix
/// <summary>
/// Adds a staged fragment to an already registered submodel, identified by submodel id.
/// Fragments are applied in registration order during <see cref="Build"/>.
/// </summary>
/// <param name="submodelId">The identifier of the target submodel.</param>
/// <param name="configure">Callback that contributes submodel elements.</param>
/// <returns>The current <see cref="IAasBuilder"/> instance for fluent chaining.</returns>
/// <exception cref="ArgumentException">Thrown when <paramref name="submodelId"/> is null, empty, or whitespace.</exception>
- /// <summary>
- /// Stages a submodel fragment configuration to be applied to the submodel with the specified id during Build.
- /// </summary>
- /// <param name="submodelId">Identifier of the target submodel; must not be null, empty, or whitespace.</param>
- /// <param name="configure">Callback that receives a <see cref="SubmodelFragmentBuilder"/> to configure the fragment.</param>
- /// <returns>The current <see cref="IAasBuilder"/> instance for fluent chaining.</returns>
- /// <exception cref="ArgumentException">Thrown when <paramref name="submodelId"/> is null, empty, or whitespace.</exception>
- /// <exception cref="ArgumentNullException">Thrown when <paramref name="configure"/> is null.</exception>
+ /// <exception cref="ArgumentNullException">Thrown when <paramref name="configure"/> is null.</exception>
public IAasBuilder AddSubmodelFragment(string submodelId, Action<SubmodelFragmentBuilder> configure)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@FluentAAS/FluentAAS.Builder/AasBuilder.cs` around lines 120 - 136, Remove the
duplicate/malformed XML documentation above the AddSubmodelFragment method: keep
a single well-formed /// <summary> block describing that it stages a submodel
fragment for Build, one set of /// <param name="submodelId"> and /// <param
name="configure"> entries, and the correct /// <returns> and /// <exception>
tags (ArgumentException for null/empty/whitespace submodelId and
ArgumentNullException for null configure). Ensure the remaining XML references
the method signature (AddSubmodelFragment), the SubmodelFragmentBuilder type,
and IAasBuilder return type, and that there are no repeated or overlapping
docblocks.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
FluentAAS/FluentAAS.Builder.Tests/AasBuilderTests.cs (1)
628-690: Duplicate shell-reference scenarios can be consolidated.Line 628 and Line 659 repeat behavior already covered at Line 303 and Line 270. Consider collapsing these into a single pair of tests (or a
[Theory]) to reduce maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@FluentAAS/FluentAAS.Builder.Tests/AasBuilderTests.cs` around lines 628 - 690, These two tests Build_WithShellReferencingUnknownSubmodel_ShouldThrowInvalidOperationException and Build_WithShellReferencingKnownSubmodel_ShouldSucceed duplicate scenarios already covered earlier; refactor by consolidating into one test (or a [Theory]) that parametrizes the submodel existence case: use CreateSut(), AddSubmodel(submodel) only for the "known" case, always AddShellInternal(shell) with the same Reference construction, then call builder.Build() and assert either that it throws InvalidOperationException containing the unknownSubmodelId or that the resulting env contains the submodel and shell. Keep references to AddShellInternal, AddSubmodel, CreateSut, and Build to locate and update the existing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@FluentAAS/FluentAAS.Builder.Tests/AasBuilderTests.cs`:
- Around line 333-354: The test asserts rollback on partial fragment application
but AasBuilder currently mutates submodels in-place; update AasBuilder.Build to
apply fragment batches transactionally by either applying fragments to a
temporary Submodel copy (or staging new elements) and only committing them into
the target Submodel when all fragments succeed, or by recording applied element
identifiers during the batch and removing them on any failure; specifically
modify AasBuilder.Build (and any helper methods used to apply fragments) to
stage changes and commit/rollback so that repeated Build() calls do not leave
duplicate SubmodelElements (fixes
Build_WhenFragmentBatchFails_ShouldRollbackAppliedFragmentsBeforeRetry).
---
Nitpick comments:
In `@FluentAAS/FluentAAS.Builder.Tests/AasBuilderTests.cs`:
- Around line 628-690: These two tests
Build_WithShellReferencingUnknownSubmodel_ShouldThrowInvalidOperationException
and Build_WithShellReferencingKnownSubmodel_ShouldSucceed duplicate scenarios
already covered earlier; refactor by consolidating into one test (or a [Theory])
that parametrizes the submodel existence case: use CreateSut(),
AddSubmodel(submodel) only for the "known" case, always AddShellInternal(shell)
with the same Reference construction, then call builder.Build() and assert
either that it throws InvalidOperationException containing the unknownSubmodelId
or that the resulting env contains the submodel and shell. Keep references to
AddShellInternal, AddSubmodel, CreateSut, and Build to locate and update the
existing tests.
🪄 Autofix (Beta)
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
Run ID: 667079b5-33b4-4b26-9dc3-ae9e86346b9e
📒 Files selected for processing (3)
FluentAAS/FluentAAS.Builder.Tests/AasBuilderTests.csFluentAAS/FluentAAS.Builder.Tests/SubModel/SubmodelFragmentBuilderTests.csREADME.md
✅ Files skipped from review due to trivial changes (1)
- README.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@FluentAAS/FluentAAS.Builder/AasBuilder.cs`:
- Around line 83-94: The XML docs on AddExistingSubmodel declare an
ArgumentException for invalid submodel.Id but the
IAasBuilder.AddExistingSubmodel interface doc only mentions
ArgumentNullException; update the interface documentation to match the
implementation by adding the ArgumentException case to
IAasBuilder.AddExistingSubmodel (or alternatively remove the ArgException doc
from AasBuilder if you intend the interface not to expose it), ensuring the
behavior described by AddSubmodelInternal and AddExistingSubmodel is consistent
across both declaration and implementation.
- Around line 144-155: Before applying staged fragments in Build(), snapshot the
current state of each affected Submodel so you can restore on failure: capture a
deep-copy (or immutable snapshot) of the Submodel elements for each submodel
identified by keys in _submodelFragments (use
_submodels.OfType<Submodel>().FirstOrDefault(...) to locate them as currently
done). Wrap the foreach over _submodelFragments where applyFragment(submodel) is
invoked in a try/catch; on exception restore every Submodel from the snapshots
to its pre-apply state and rethrow the exception (do not clear
_submodelFragments on failure so retries still have the staged fragments), and
only clear _submodelFragments after the loop completes successfully.
🪄 Autofix (Beta)
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
Run ID: 01f9806f-ff58-4a26-bbcf-3cc31ddfd287
📒 Files selected for processing (4)
FluentAAS/FluentAAS.Builder.Tests/AasBuilderTests.csFluentAAS/FluentAAS.Builder/AasBuilder.csFluentAAS/FluentAAS.Builder/IAasBuilder.csFluentAAS/FluentAAS.Builder/SubModel/SubmodelFragmentBuilder.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- FluentAAS/FluentAAS.Builder/IAasBuilder.cs
Comment on lines
+83
to
94
| /// <summary> | ||
| /// Registers an existing <see cref="Submodel"/> instance with the builder without requiring an <c>IdShort</c> value. | ||
| /// </summary> | ||
| /// <param name="submodel">The submodel to register; its <c>Id</c> must be set (non-empty).</param> | ||
| /// <returns>The current builder instance for fluent chaining.</returns> | ||
| /// <exception cref="ArgumentNullException">Thrown when <paramref name="submodel"/> is null.</exception> | ||
| /// <exception cref="ArgumentException">Thrown when <paramref name="submodel"/> has a null, empty, or whitespace <c>Id</c>.</exception> | ||
| public IAasBuilder AddExistingSubmodel(Submodel submodel) | ||
| { | ||
| AddSubmodelInternal(submodel); | ||
| return this; | ||
| } |
Contributor
There was a problem hiding this comment.
Interface contract divergence: ArgumentException not declared in interface.
The implementation documents ArgumentException (line 89) for invalid Id, but IAasBuilder.AddExistingSubmodel (per context snippet 3) only declares ArgumentNullException. Consider updating the interface documentation to include the ArgumentException case for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@FluentAAS/FluentAAS.Builder/AasBuilder.cs` around lines 83 - 94, The XML docs
on AddExistingSubmodel declare an ArgumentException for invalid submodel.Id but
the IAasBuilder.AddExistingSubmodel interface doc only mentions
ArgumentNullException; update the interface documentation to match the
implementation by adding the ArgumentException case to
IAasBuilder.AddExistingSubmodel (or alternatively remove the ArgException doc
from AasBuilder if you intend the interface not to expose it), ensuring the
behavior described by AddSubmodelInternal and AddExistingSubmodel is consistent
across both declaration and implementation.
Freezor
deleted the
12-enable-incremental-submodel-composition-via-addsubmodel-at-any-stage-of-the-build-process
branch
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.
… (#19)
Update README for staged submodel composition and remove extra spec doc
Fix staged fragment idempotency and preserve internal submodel compatibility
Summary by CodeRabbit