Conversation
|
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 2 minutes and 41 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 (8)
WalkthroughAdds a new TechnicalData builder and templates: fluent APIs to compose motor performance and bearing characteristics submodels, an internal ECLASS property catalog and validators, semantic/identifier constants, an example composition, and tests covering validation and construction. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ShellBuilder
participant TechDataBuilder as TechnicalDataBuilder
participant Catalog as EclassCatalog
participant SubmodelBuilder
participant Shell
Client->>ShellBuilder: AddTechnicalData(id, idShort)
ShellBuilder->>TechDataBuilder: instantiate(builder)
Client->>TechDataBuilder: WithMotorPerformance(configure)
Client->>TechDataBuilder: WithBearingCharacteristics(configure)
Note over TechDataBuilder: store group assignments and semantics
Client->>TechDataBuilder: BuildTechnicalData()
TechDataBuilder->>TechDataBuilder: validate required groups/properties
TechDataBuilder->>Catalog: ForIdShort(propertyId)
Catalog-->>TechDataBuilder: PropertyDefinition
TechDataBuilder->>Catalog: IsKnownEclassReference(irdi)
Catalog-->>TechDataBuilder: bool
alt Validation fails
TechDataBuilder-->>Client: InvalidOperationException
else Validation succeeds
TechDataBuilder->>SubmodelBuilder: create submodel & collections
loop For each group & property
SubmodelBuilder->>SubmodelBuilder: Add Property (value, unit, semanticId)
end
SubmodelBuilder->>Shell: AddSubmodelReference(submodel)
TechDataBuilder-->>Client: return ShellBuilder
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related issues
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
FluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataBuilder.cs (1)
85-88: Redundant validation:ValidateEclassCatalogMappingalways passes.The
assignment.Definitionoriginates fromTechnicalDataEclassCatalog.ForIdShort()inUpsertDecimalProperty, soValidateEclassCatalogMapping(assignment.Definition.EclassIrdi)will always succeed—the IRDI is guaranteed to exist in the catalog.♻️ Consider removing redundant validation
foreach (var assignment in group.Assignments.Values.OrderBy(x => x.Definition.IdShort, StringComparer.Ordinal)) { - ValidateEclassCatalogMapping(assignment.Definition.EclassIrdi); - var property = new Property(assignment.Definition.ValueType)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@FluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataBuilder.cs` around lines 85 - 88, The call to ValidateEclassCatalogMapping(assignment.Definition.EclassIrdi) is redundant because assignment.Definition is created via TechnicalDataEclassCatalog.ForIdShort() in UpsertDecimalProperty and therefore always has a valid EclassIrdi; remove the redundant validation call from the foreach loop in TechnicalDataBuilder (i.e., delete the ValidateEclassCatalogMapping(...) invocation referencing assignment.Definition.EclassIrdi) and run tests to ensure no other code relies on its side effects.
🤖 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.Templates.Tests/TechnicalData/TechnicalDataBuilderTests.cs`:
- Around line 92-101: The test CompositionExample_ShouldBuildEnvironment fails
because TechnicalDataCompositionExample.BuildMotorAndBearingExampleEnvironment
calls an IShellBuilder.Build() method that doesn't exist; update the builder
usage in TechnicalDataCompositionExample so it calls the correct method on
IShellBuilder (or add a Build() implementation to the IShellBuilder concrete
builder) to return the built shell/environment, ensuring the call signature and
return type match the rest of the composition code used by
CompositionExample_ShouldBuildEnvironment and any related methods that expect
the built result.
- Around line 83-89: The test currently asserts the deprecated Property.Category
("ratedVoltage.Category.ShouldBe(\"V\")"); update the test and builder so the
unit is stored on the ConceptDescription's DataSpecificationIec61360 instead:
remove the Category assertion in
FluentAAS.Templates.Tests.TechnicalData.TechnicalDataBuilderTests (the
ratedVoltage variable usage) and add an assertion that the ConceptDescription
created by TechnicalDataBuilder contains a DataSpecificationIec61360 with unit
== "V"; update TechnicalDataBuilder (the code that currently sets
Property.Category around the TechnicalDataBuilder.Create... logic) to stop
populating Property.Category and instead populate the
ConceptDescription.DataSpecificationIec61360.unit field with the unit string.
In `@FluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataBuilder.cs`:
- Around line 89-96: The Property construction incorrectly stores measurement
units in Category; update TechnicalDataBuilder to remove Category =
assignment.Unit and instead attach a DataSpecificationIec61360 to the created
Property (use the Property instance created in TechnicalDataBuilder.cs)
populating the unit (assignment.Unit) and unitId (use your ReferenceFactory to
create a global unit Reference) and keep SemanticId/ValueId as-is; if a
classification is required set Category to an appropriate AAS classification
token (e.g., "PARAMETER") rather than the unit.
In
`@FluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataCompositionExample.cs`:
- Around line 9-23: BuildTechnicalData() currently returns IShellBuilder which
lacks Build(), causing the fluent chain to fail; change BuildTechnicalData() to
return an interface that exposes the environment-level Build() (e.g., return
IEnvironmentBuilder or a new composite interface exposing Build()), update the
implementation of BuildTechnicalData() to hand back the environment builder
instance instead of IShellBuilder, and adjust any interface definitions and
callers (references: BuildTechnicalData(), IShellBuilder, IEnvironmentBuilder,
Build()) so the final .Build() call is available in the fluent chain.
---
Nitpick comments:
In `@FluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataBuilder.cs`:
- Around line 85-88: The call to
ValidateEclassCatalogMapping(assignment.Definition.EclassIrdi) is redundant
because assignment.Definition is created via
TechnicalDataEclassCatalog.ForIdShort() in UpsertDecimalProperty and therefore
always has a valid EclassIrdi; remove the redundant validation call from the
foreach loop in TechnicalDataBuilder (i.e., delete the
ValidateEclassCatalogMapping(...) invocation referencing
assignment.Definition.EclassIrdi) and run tests to ensure no other code relies
on its side effects.
🪄 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: 67e2420c-eb49-45e5-9e77-0fb557ec51f2
📒 Files selected for processing (7)
FluentAAS/FluentAAS.Templates.Tests/TechnicalData/TechnicalDataBuilderTests.csFluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataBuilder.csFluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataCompositionExample.csFluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataEclassCatalog.csFluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataIdentifiers.csFluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataSemantics.csFluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataShellBuilderExtensions.cs
| var ratedVoltage = motorGroup.Value! | ||
| .OfType<Property>() | ||
| .Single(x => x.IdShort == TechnicalDataIdentifiers.RatedVoltage); | ||
|
|
||
| ratedVoltage.Value.ShouldBe("400"); | ||
| ratedVoltage.Category.ShouldBe("V"); | ||
| ratedVoltage.SemanticId!.Keys.Single().Value.ShouldBe(TechnicalDataSemantics.RatedVoltage); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how Category is typically used in the AAS 3.0 library and existing codebase
rg -n -C3 'Category\s*=' --type=csRepository: Freezor/FluentAAS
Length of output: 3003
🌐 Web query:
AAS 3.0 Property Category field usage and IEC 61360 unit storage
💡 Result:
In AAS 3.0, the 'category' field is defined in the Referable abstract class, inherited by elements like Property. It provides meta-information about the element's class, affecting expected attributes and constraints (e.g., distinguishing measurement values). It is optional (string type, default null) and deprecated since V3.0, though still present for backward compatibility. Legacy values include PARAMETER, CONSTANT, VALUE for data elements like Properties, influencing validation (e.g., in ConceptDescription constraints referencing category, marked deprecated). IEC 61360 units for quantitative properties (e.g., dataType INTEGER_MEASURE, REAL_MEASURE) are stored in ConceptDescriptions via the DataSpecificationIec61360 template, using 'unit' (string, primary unit text) or 'unitId' (Reference to global unit identifier, e.g., IRDI). These are embedded in the semanticId-referenced ConceptDescription of the Property, not directly in the Property.value. Constraint AASc-3a-009 requires unit/unitId for measure types. The Property.valueType maps IEC data types to XSD (e.g., xs:integer for INTEGER_MEASURE).
Citations:
- 1: https://aas-core-works.github.io/aas-core3.0rc02-typescript/classes/types.Property.html
- 2: https://aas-core-works.github.io/aas-core-meta/v3/Property.html
- 3: https://aas-core-works.github.io/aas-core-meta/v3/ConceptDescription.html
- 4: https://industrialdigitaltwin.org/en/wp-content/uploads/sites/2/2023/04/IDTA-01003-a-3-0_SpecificationAssetAdministrationShell_Part3a_DataSpecification_IEC61360.pdf
- 5: https://industrialdigitaltwin.io/aas-specifications/IDTA-01003-a/v3.1.1/specification.html
- 6: https://industrialdigitaltwin.io/aas-specifications/IDTA-01001/v3.1.1/index.html
- 7: https://industrialdigitaltwin.io/aas-specifications/IDTA-01001/v3.1/annex/general.html
- 8: https://aas-core-works.github.io/aas-core3.0rc02-typescript/interfaces/types.IReferable.html
- 9: https://aas-core-works.github.io/aas-core-meta/v3/Reference.html
- 10: https://aas-core-works.github.io/aas-core-meta/v3/Referable.html
Remove Category field usage for unit storage; use ConceptDescription's DataSpecificationIec61360 instead.
The test asserts ratedVoltage.Category.ShouldBe("V"), but in AAS 3.0, the Category field is deprecated and meant for element classification (e.g., PARAMETER, CONSTANT, VALUE). Units should be stored in the ConceptDescription's DataSpecificationIec61360 template via the unit field, not in the Property's Category field. This applies to the builder implementation as well (TechnicalDataBuilder.cs:95).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@FluentAAS/FluentAAS.Templates.Tests/TechnicalData/TechnicalDataBuilderTests.cs`
around lines 83 - 89, The test currently asserts the deprecated
Property.Category ("ratedVoltage.Category.ShouldBe(\"V\")"); update the test and
builder so the unit is stored on the ConceptDescription's
DataSpecificationIec61360 instead: remove the Category assertion in
FluentAAS.Templates.Tests.TechnicalData.TechnicalDataBuilderTests (the
ratedVoltage variable usage) and add an assertion that the ConceptDescription
created by TechnicalDataBuilder contains a DataSpecificationIec61360 with unit
== "V"; update TechnicalDataBuilder (the code that currently sets
Property.Category around the TechnicalDataBuilder.Create... logic) to stop
populating Property.Category and instead populate the
ConceptDescription.DataSpecificationIec61360.unit field with the unit string.
| var property = new Property(assignment.Definition.ValueType) | ||
| { | ||
| IdShort = assignment.Definition.IdShort, | ||
| Value = assignment.Value, | ||
| ValueId = ReferenceFactory.GlobalSemanticReference(assignment.Definition.EclassIrdi), | ||
| SemanticId = ReferenceFactory.GlobalConceptDescription(assignment.Definition.EclassIrdi), | ||
| Category = assignment.Unit | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
IDTA Technical Data submodel template property unit storage AAS 3.0
💡 Result:
The IDTA Technical Data submodel template is the "Generic Frame for Technical Data for Industrial Equipment in Manufacturing" (IDTA number 02003, version 2.0), published by the Industrial Digital Twin Association (IDTA) for use with Asset Administration Shell (AAS) 3.0. It provides an interoperable structure for technical properties of assets, using semantic dictionaries like ECLASS and IEC CDD. Properties in this submodel are defined as AAS Property elements with semanticIds referencing external concept descriptions (e.g., IRDIs from ECLASS). For quantitative properties (e.g., dataType INTEGER_MEASURE, REAL_MEASURE), the unit is specified via the IEC 61360 data specification template (IDTA-01003-a), which includes attributes: - unit: string for unit in text (e.g., "m³" for storage volume). - unitId: Reference to a unit concept (e.g., IRDI for cubic meter). "Storage" likely refers to a property like storage capacity (e.g., ECLASS property for volume), where the value has a unit such as cubic meters (m³). Example structure: - Property idShort: "storageCapacity" - semanticId: IRDI (e.g., 0173-1#02-BAF188#007 for gross volume) - value: e.g., "10" - hasDataSpecification: DataSpecificationIec61360 with unit="m³", unitId=reference to unit concept. This follows AAS 3.0 metamodel (IDTA-01001), where units are part of embedded data specs for interoperability. Official spec downloadable from IDTA site; GitHub repo at admin-shell-io/submodel-templates for JSON/UML/AASX files.
Citations:
- 1: https://industrialdigitaltwin.org/en/content-hub/submodels
- 2: https://www.scribd.com/document/961336334/IDTA-02003-Generic-Frame-for-Technical-Data
- 3: https://eclass.eu/zh/%E6%96%B0%E9%97%BB/%E6%96%B0%E9%97%BB/new-version-of-the-idta-submodel-technical-data-structure-and-use-of-the-submodel-with-eclass
- 4: https://industrialdigitaltwin.io/aas-specifications/IDTA-01003-a/v3.1.1/specification.html
- 5: https://industrialdigitaltwin.org/en/content-hub
🏁 Script executed:
# First, find the Property class definition
find . -type f -name "*.cs" | head -20Repository: Freezor/FluentAAS
Length of output: 1180
🏁 Script executed:
# Search for Property class definition
rg "class Property" -A 10 --max-count 5Repository: Freezor/FluentAAS
Length of output: 43
🏁 Script executed:
# Look for IEC 61360 or DataSpecification references related to units
rg -i "iec.?61360|dataspecification|unit" --max-count 20Repository: Freezor/FluentAAS
Length of output: 10802
🏁 Script executed:
# Check if there are comments explaining Category usage in this file
cat -n FluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataBuilder.cs | head -100Repository: Freezor/FluentAAS
Length of output: 4192
🏁 Script executed:
# Search for other Category assignments to see if this is a pattern
rg "Category\s*=" -B 2 -A 2 --max-count 10Repository: Freezor/FluentAAS
Length of output: 2220
Store unit information using IEC 61360 data specification instead of the Category field.
The Category field per AAS 3.0 should contain classification values (e.g., PARAMETER, CONSTANT, VARIABLE), not measurement units. IDTA Technical Data (IDTA-02003 v2.0) specifies units must be stored via DataSpecificationIec61360 with unit (string) and unitId (Reference) attributes. The current implementation on lines 89–96 violates this requirement by assigning the unit string directly to Category. This breaks IDTA compliance and causes misinterpretation of property semantics by AAS consumers.
Consider:
- Adding
DataSpecificationIec61360(or embedded data specification) to thePropertywith properunitandunitIdfields - Removing
Category = assignment.Unitor setting it to an appropriate classification value if needed
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@FluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataBuilder.cs` around
lines 89 - 96, The Property construction incorrectly stores measurement units in
Category; update TechnicalDataBuilder to remove Category = assignment.Unit and
instead attach a DataSpecificationIec61360 to the created Property (use the
Property instance created in TechnicalDataBuilder.cs) populating the unit
(assignment.Unit) and unitId (use your ReferenceFactory to create a global unit
Reference) and keep SemanticId/ValueId as-is; if a classification is required
set Category to an appropriate AAS classification token (e.g., "PARAMETER")
rather than the unit.
![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: a7c7dd06b9
ℹ️ 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".
| .WithWidth(20m, "mm", TechnicalDataSemantics.Width) | ||
| .WithLimitingSpeed(6500m, "1/min", TechnicalDataSemantics.LimitingSpeed)) | ||
| .BuildTechnicalData() | ||
| .Build(); |
There was a problem hiding this comment.
BuildTechnicalData() returns IShellBuilder, but IShellBuilder has no Build() method (it only exposes CompleteShellConfiguration() and Parent). Chaining .Build() directly here makes this example uncompilable and breaks CompositionExample_ShouldBuildEnvironment, so callers cannot execute the sample as written unless they first return to AasBuilder.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
FluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataCompositionExample.cs (1)
11-12: Consider extracting sample identifiers to named constants for maintainability.The URNs and display name are currently inline literals; moving them to local constants would reduce typo risk if this sample evolves.
♻️ Suggested refactor
public static IEnvironment BuildMotorAndBearingExampleEnvironment() { + const string aasId = "urn:aas:example:drivetrain:1000"; + const string aasName = "DriveTrainAAS"; + const string technicalDataSubmodelId = "urn:aas:submodel:technical-data:drivetrain:1000"; + var aasBuilder = AasBuilder.Create(); - aasBuilder.AddShell("urn:aas:example:drivetrain:1000", "DriveTrainAAS") - .AddTechnicalData("urn:aas:submodel:technical-data:drivetrain:1000") + aasBuilder.AddShell(aasId, aasName) + .AddTechnicalData(technicalDataSubmodelId) .WithMotorPerformance(motor => motor🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@FluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataCompositionExample.cs` around lines 11 - 12, Extract the inline literal identifiers used in the aasBuilder.AddShell(...).AddTechnicalData(...) chain into local named constants (e.g., const string ShellId, const string ShellDisplayName, const string TechnicalDataSubmodelId) and replace the raw literal strings with those constants; update the AddShell call to use ShellId and ShellDisplayName and the AddTechnicalData call to use TechnicalDataSubmodelId so the identifiers are centralized and less error-prone.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@FluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataCompositionExample.cs`:
- Around line 11-12: Extract the inline literal identifiers used in the
aasBuilder.AddShell(...).AddTechnicalData(...) chain into local named constants
(e.g., const string ShellId, const string ShellDisplayName, const string
TechnicalDataSubmodelId) and replace the raw literal strings with those
constants; update the AddShell call to use ShellId and ShellDisplayName and the
AddTechnicalData call to use TechnicalDataSubmodelId so the identifiers are
centralized and less error-prone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a044ec3-2422-4aed-bbfd-69ee42769d53
📒 Files selected for processing (1)
FluentAAS/FluentAAS.Templates/TechnicalData/TechnicalDataCompositionExample.cs
Freezor
deleted the
codex/implement-technicaldatabuilder-for-aas-project
branch
1 participant
Motivation
Description
TechnicalDataBuilderwith typed groupsMotorPerformanceandBearingCharacteristicsand group-level builders such asMotorPerformanceGroupBuilderandBearingCharacteristicsGroupBuilder.TechnicalDataEclassCatalogand semantics/constants inTechnicalDataSemanticsandTechnicalDataIdentifiersthat use concrete IRDIs (e.g.,0173-1#02-BAF053#008forRatedVoltage).V,A,kW,mm,1/min), and semantic-ID/ECLASS mapping conformity, and wire-up to AAS elements using existingReferenceFactoryhelpers.AddTechnicalDataextension forIShellBuilder, a runnable composition exampleTechnicalDataCompositionExample.BuildMotorAndBearingExampleEnvironment(), and unit tests underFluentAAS.Templates.Tests/TechnicalDataexercising required-parameter, unit-conformity, semantic-ID validation and a positive build path.Testing
FluentAAS.Templates.Tests/TechnicalData/TechnicalDataBuilderTests.cs) and cover the three validation categories plus a happy-path composition and example execution.dotnet testforFluentAAS.Templates.Tests, but execution could not be completed becausedotnetis not available in the environment where the rollout ran.AddTechnicalDataand was implemented to match project naming and builder patterns so it should compile and run in the normal project toolchain.Codex Task
Summary by CodeRabbit
New Features
Tests