fixing namespace for relationships and making it backwards compatible… by martafullen · Pull Request #197 · eclipse-aaspe/package-explorer

martafullen · 2024-04-11T19:47:01Z

… (#690)

AlexanderWollbrink

my fix regarding the relationship namespaces, that falsely didn't get taken over to Eclipse. Thank you for cherry picking @martafullen

juileetikekar

In addition to my comments,

I could find one more place for a namespace change:
"http://www.admin-shell.io/aasx/relationships/aas-suppl"

Shouldn't these also be changed?

src/AasxCsharpLibrary/AdminShellPackageEnv.cs

AlexanderWollbrink · 2024-04-15T11:55:09Z

The fix for the bug is up.
I also added backwards compatible reading for "http://www.admin-shell.io/aasx/relationships/aas-suppl". Unfortunately, I can not see where the relationship for this is created, so I wasn't able to "convert" it to the new namespace. Maybe you could look into this @juileetikekar?

juileetikekar · 2024-04-15T13:06:14Z

Hi @AlexanderWollbrink,

Thank you for the fix.

Regarding fix related to "aas-suppl" file, please find my observations below:

When creating a new AASX V3 file => A correct new namespace gets added.
When converting V2 -> V3 => the namespace does not change, i.e., it remains "http://www.admin-shell.io/aasx/relationships/aas-suppl" (see the screenshot below).

The relationship gets created in case of supplementary files. If the AASX package has a supplimentary file, the relationship is found under "/aasx//_rels/<AAS_id>.aas.xml.rel"

You can use the below AASX file to reproduce the issue. Please rename the from from .zip to .aasx.

TestAAS_V2.zip

If you unzip this file, you will find "AssetAdministrationShell---0F98619F.aas.xml.rels" file, which consists of "aas_suppl" relationship under "\aasx\AssetAdministrationShell---0F98619F\ _rels".

Please let me know if you need more information.

AlexanderWollbrink · 2024-04-15T14:23:25Z

Hey @juileetikekar,
thank you for the answer.
My approach would be, to find the "old" namespace relationships, delete those and replace them with the new ones.
The issue is, that I can't find where in the code the existing relationships for suppl. files appear or where inside the package-structure they are saved.

juileetikekar · 2024-04-15T16:53:58Z

Hi @AlexanderWollbrink,

In my opinion, we will need to add a code explicitely to handle the namespace change. The current implementation just takes care of "aas_suppl" relationship only in case of adding and deleting file in case of "save" operation.

AlexanderWollbrink · 2024-04-16T09:33:00Z

I tested this with several AASX files that I had and it worked nicely for all the relationships.
In my opinion we can merge this into main.

Freezor

In general the changes look fine to me. There are some smaller suggestions from my side. There still would be a lot to improve, but that could also mean to refactor that whole class.
I recommend, that we create unit tests for this class in the near future. That would really help us.

Freezor · 2024-04-16T11:48:58Z

src/AasxCsharpLibrary/AdminShellPackageEnv.cs

+                    foreach (var x in xs)
+                        if (x.SourceUri.ToString() == "/")
+                        {
+                            //originPart = package.GetPart(x.TargetUri);


nitpicking remove commented code

Freezor · 2024-04-16T11:54:32Z

src/AasxCsharpLibrary/AdminShellPackageEnv.cs

+                            break;
+                        }
+                    if (originPart == null)
+                        throw (new Exception("Unable to find AASX origin. Aborting!"));


optional: I know it is commonly used here, but normally we should not throw the generic Exception, but use a more specific Exception, like ArgumentNullException, or create in rare cases our own Exception implementations.
further infos

Freezor · 2024-04-16T11:56:06Z

src/AasxCsharpLibrary/AdminShellPackageEnv.cs

+                //if its still null, then try with the old
+                //this is to make it backwards compatible
+                if (specPart == null) { 
+                    xs = originPart.GetRelationshipsByType("http://www.admin-shell.io/aasx/relationships/aas-spec");


The url "http://www.admin-shell.io/aasx/relationships/aas-spec" is used in multiple locations. sometimes with and without the www prefix. The code could be improved by using two const values, so you make an issue only in one location

Freezor · 2024-04-16T11:56:30Z

src/AasxCsharpLibrary/AdminShellPackageEnv.cs

+                    xs = originPart.GetRelationshipsByType("http://www.admin-shell.io/aasx/relationships/aas-spec");
+                    foreach (var x in xs)
+                    {
+                        //specPart = package.GetPart(x.TargetUri);


Suggested change

//specPart = package.GetPart(x.TargetUri);

Freezor · 2024-04-16T11:56:56Z

src/AasxCsharpLibrary/AdminShellPackageEnv.cs

+                    foreach (var x in xs)
+                        if (x.SourceUri.ToString() == "/")
+                        {
+                            //originPart = package.GetPart(x.TargetUri);


Suggested change

//originPart = package.GetPart(x.TargetUri);

Freezor · 2024-04-16T12:00:38Z

src/AasxCsharpLibrary/AdminShellPackageEnv.cs

+                        originPart.DeleteRelationship(x.Id);
+                        originPart.CreateRelationship(
+                            specPart.Uri, TargetMode.Internal,
+                            "http://admin-shell.io/aasx/relationships/aas-spec");


The code snippet at lines 796-798, where DeleteRelationship and CreateRelationship methods are called, appears to be duplicated elsewhere in the class. Consider refactoring this repetitive logic into a separate method to reduce duplication. Although I understand that the class is already substantial in size, extracting this logic into a method would improve readability and maintainability.

IDEA

// Refactored method to handle deletion and creation of relationships private void DeleteAndCreateRelationship(string id, Uri specPartUri) { originPart.DeleteRelationship(id); originPart.CreateRelationship(specPartUri, TargetMode.Internal, "http://admin-shell.io/aasx/relationships/aas-spec"); } // Usage of the refactored method DeleteAndCreateRelationship(x.Id, specPart.Uri);

Freezor · 2024-04-16T12:03:30Z

src/AasxCsharpLibrary/AdminShellPackageEnv.cs

                    {
                        // get the supplementaries from the package, derived from spec
                        xs = specPart.GetRelationshipsByType("http://www.admin-shell.io/aasx/relationships/aas-suppl");
+                        if(xs == null) xs = specPart.GetRelationshipsByType("http://www.admin-shell.io/aasx/relationships/aas-suppl");


The code above is used multiple times across the codebase for retrieving relationships by type and handling a fallback scenario could benefit from a method abstraction to adhere to the DRY principle. Consider creating a method to encapsulate this logic, promoting code reuse and improving maintainability.

Revised version of the code snippet:

// Method to retrieve relationships by type with fallback private IEnumerable<Relationship> GetRelationshipsByTypeWithFallback(Part specPart, string relationshipType) { var relationships = specPart.GetRelationshipsByType(relationshipType); if (relationships == null) { relationships = specPart.GetRelationshipsByType(relationshipType); } return relationships; } // Usage of the refactored method var xs = GetRelationshipsByTypeWithFallback(specPart, "http://www.admin-shell.io/aasx/relationships/aas-suppl");

Freezor · 2024-04-16T12:10:26Z

src/AasxCsharpLibrary/AdminShellPackageEnv.cs

+                    xs = package.GetRelationshipsByType(
+                        "http://www.admin-shell.io/aasx/relationships/aasx-origin");
+                    foreach (var x in xs)
+                        if (x.SourceUri.ToString() == "/")


I strongly recommend to use the Equals comparison method for strings.

Suggested change

if (x.SourceUri.ToString() == "/")

if (x.SourceUri.Equals("/"))

== Operator: The == operator compares the references of the string objects. If two strings have the same sequence of characters, but they are stored in different memory locations.

Equals() Method: The Equals() method, on the other hand, compares the actual contents of the strings. It checks whether the sequences of characters in both strings are identical.

mristin · 2024-04-16T12:15:49Z

Just a heads-up: there is a unit tested & battle tested implementation of AASX package: https://github.com/aas-core-works/aas-package3-csharp

You might want to consider using it in the future.

juileetikekar · 2024-04-17T06:43:15Z

My suggestion at this point is to merge the PR into main to unblock other activities. We can continue with the refactoring in next step.

@AlexanderWollbrink could you please merge the PR into main?

fixing namespace for relationships and making it backwards compatible…

b75f1d5

… (#690)

martafullen requested review from AlexanderWollbrink and juileetikekar April 11, 2024 19:47

AlexanderWollbrink approved these changes Apr 12, 2024

View reviewed changes

juileetikekar reviewed Apr 12, 2024

View reviewed changes

src/AasxCsharpLibrary/AdminShellPackageEnv.cs Show resolved Hide resolved

src/AasxCsharpLibrary/AdminShellPackageEnv.cs Show resolved Hide resolved

src/AasxCsharpLibrary/AdminShellPackageEnv.cs Show resolved Hide resolved

added fix for bug regarding two relationship entries

0cb0740

Support for "aas-suppl" from V2 to V3

12312e9

Freezor approved these changes Apr 16, 2024

View reviewed changes

juileetikekar approved these changes Apr 17, 2024

View reviewed changes

AlexanderWollbrink merged commit 003f5c3 into main Apr 17, 2024

AlexanderWollbrink deleted the namespace branch April 17, 2024 07:20

	if (x.SourceUri.ToString() == "/")
	if (x.SourceUri.Equals("/"))

Conversation

martafullen commented Apr 11, 2024

Uh oh!

AlexanderWollbrink left a comment

Choose a reason for hiding this comment

Uh oh!

juileetikekar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexanderWollbrink commented Apr 15, 2024

Uh oh!

juileetikekar commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexanderWollbrink commented Apr 15, 2024

Uh oh!

juileetikekar commented Apr 15, 2024

Uh oh!

AlexanderWollbrink commented Apr 16, 2024

Uh oh!

Freezor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mristin commented Apr 16, 2024

Uh oh!

juileetikekar commented Apr 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

juileetikekar left a comment •

edited

Loading

juileetikekar commented Apr 15, 2024 •

edited

Loading