Repository navigation
Fix #389: PIM <-> PSM data mapping - #391
antoineatstariongroup merged 3 commits into
Conversation
samatstariongroup
left a comment
samatstariongroup
left a comment
There was a problem hiding this comment.
Review of the mapping added on top of #390, checked against Resources/ptc-25-02-30.json. The solution builds and all tests pass locally on 50caf2d4; the problems below are cases the tests do not cover.
Functional problems
1. A live branch or tag maps to one deleted on 0001-01-01
Branch.FromPim and Tag.FromPim (SysML2.NET/PSM/DTO/Mapping/Branch.Mapping.cs, SysML2.NET/PSM/DTO/Mapping/Tag.Mapping.cs) copy the PIM Deleted straight into the nullable PSM Deleted. The PIM type is a non-nullable DateTime, so a branch that was never deleted arrives as default and leaves as a non-null timestamp. ToPim does the reverse (null → default), so PSM → PIM → PSM is not an identity.
The round-trip tests only cover a record with Deleted set. The root cause is the non-nullable Deleted on the PIM DTO, already raised on #390.
2. Guid.Empty leaks into requests as a real reference
The request ToPim methods use ?? Guid.Empty for absent references, and FromPim copies the PIM value back without the reverse check. A PIM Project with no default branch therefore produces a ProjectRequest with a non-null all-zero DefaultBranch, where the OpenAPI document makes that member optional.
The same pattern affects:
BranchRequest.HeadandBranch.HeadExternalDataRequest.IdExternalRelationshipRequest(Id,ElementEnd,ExternalDataEnd)ProjectUsageRequest(Id,UsedCommit,UsedProject)
FromPim should map Guid.Empty back to null for the nullable PSM members, with a test for the absent case of each.
Limitations to document or resolve
3. Id-only shells where the OpenAPI document requires full objects
DataDifference.FromPim builds a DataVersion with only Id set, and MergeResultMapper.ToPsm does the same for Commit and DataIdentity. The OpenAPI document requires full objects there (identity, payload, created, owningProject), so serializing these shells yields invalid responses. The PIM DTO carries only ids, so the mapper cannot do better without a lookup; the XML docs should state this, or the mapper should accept a resolver.
4. Constraint value types are lost
ConstraintMapper.ToPimValue renders numbers, booleans and ids as strings, and ToPsmValue wraps every value as a string, so 4 comes back as "4". The remarks in SysML2.NET/PSM/DTO/ConstraintMapper.cs document this, but it means a query condition does not survive a pass through the PIM.
5. No mapping for CommitRequest or DataVersionRequest
#390 added Commit.Change as the input of createCommit, but nothing maps it. Issue #389 calls the contract opt-in, so please either add the mapping or say in the PR description that it is out of scope.
Tests
6. Fixture layout
SysML2.NET.Tests/PSM/DTO/PimMappingTestFixture.cs covers some 23 production types in one fixture, with Verify…RoundTrip test names. TESTING.md asks for one fixture per production type and Verify{MethodUnderTest} names.
| /// Gets or sets the commit to which the branch is currently pointing. It represents the latest state of the | ||
| /// <see cref="Project"/> on the given branch | ||
| /// </summary> | ||
| [Property(lowerValue: 1, upperValue: 1)] |
There was a problem hiding this comment.
please use full form, the intent is to be self documenting
There was a problem hiding this comment.
The rebase took care of that
| /// <summary> | ||
| /// Gets or sets the timestamp at which the <see cref="Commit"/> was created | ||
| /// </summary> | ||
| [Property(lowerValue: 1, upperValue: 1)] |
There was a problem hiding this comment.
please use full form, the intent is to be self documenting
There was a problem hiding this comment.
The rebase took care of that
|
please document the mapping principle and difference between PIM and REST model in the wiki |
50caf2d to
1ce5a04
Compare
|
Prerequisites
Description
Fix #389
Provides datamapping between the PIM and REST PSM. PR #390 needs to be merged-in first