Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add metamodel and api classes after latest run #3

Draft
wants to merge 1 commit into
base: generated-v3-classes-and-tostring
Choose a base branch
from

Conversation

sebbader-sap
Copy link
Collaborator

Including these extensions:

  • copyconstructor
  • default values

* copyconstructor
* default values
@sebbader-sap sebbader-sap requested a review from emildinchev July 14, 2023 15:50
@sebbader-sap
Copy link
Collaborator Author

Still open:

  • Unit tests of the serialisers fail due to not yet updated "expected files"

@arnoweiss
Copy link

There's an issue with the way copy constructors are generated. Currently, for a SMEC, that looks like this:

    public DefaultSubmodelElementCollection(SubmodelElementCollection x) {
        this.embeddedDataSpecifications = x.getEmbeddedDataSpecifications();
        this.extensions = x.getExtensions();
        this.semanticId = x.getSemanticId();
        this.supplementalSemanticIds = x.getSupplementalSemanticIds();
        this.qualifiers = x.getQualifiers();
        this.category = x.getCategory();
        this.description = x.getDescription();
        this.displayName = x.getDisplayName();
        this.idShort = x.getIdShort();
        this.value = x.getValue();
    }

The copy is populated via getting the respective field from the original object. This creates only shallow copies! If a field requires something that's not either immutable or primitive, there will NOT be a new sub-object created. Meaning that copy.getDescription() will point to the exact same bytes in memory as original.getDescription().

Why this can lead to unexpected behavior as in the test below.

package org.eclipse.digitaltwin.aas4j.v3.model;


import org.eclipse.digitaltwin.aas4j.v3.model.builder.LangStringTextTypeBuilder;
import org.eclipse.digitaltwin.aas4j.v3.model.builder.PropertyBuilder;
import org.eclipse.digitaltwin.aas4j.v3.model.impl.DefaultLangStringTextType;
import org.eclipse.digitaltwin.aas4j.v3.model.impl.DefaultProperty;
import org.eclipse.digitaltwin.aas4j.v3.model.impl.DefaultSubmodelElementCollection;
import org.junit.Test;

import java.util.List;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;

public class CopyConstructorTest {
    @Test
    public void onProperty() {

        DefaultSubmodelElementCollection original = new DefaultSubmodelElementCollection.Builder()
                .value(new DefaultProperty.Builder()
                        .description(new DefaultLangStringTextType.Builder()
                                .language("en")
                                .text("MyDescription")
                                .build())
                        .build())
                .build();

        DefaultSubmodelElementCollection copy = new DefaultSubmodelElementCollection(original); // make copy

        assertEquals(getPropertyDescriptionTextFromSmec(original), getPropertyDescriptionTextFromSmec(copy)); // true
        
        copy.getValue().get(0).getDescription().get(0).setText("DifferentDescription"); // change value in copy

        assertNotEquals(
                getPropertyDescriptionTextFromSmec(original), 
                getPropertyDescriptionTextFromSmec(copy)
        );
        // Objects should be different. Copy was changed, the original was not. However, both hold "DifferentDescription".
        
    }

    private String getPropertyDescriptionTextFromSmec(SubmodelElementCollection smec) {
        String text = smec.getValue().get(0).getDescription().get(0).getText();
        System.out.println(text);
        return text;
    }
}

@sebbader-sap
Copy link
Collaborator Author

I need more time to design a deep copy pattern. Will keep this PR open as a reminder.

@@ -55,7 +55,7 @@ public static DefaultEndpoint.Builder createEndpointBuilder() {

return new DefaultEndpoint.Builder()
.protocolInformation(createProtocolInformationBuilder().build())
.withInterface(DEFAULT_INTERFACE_VALUE);
._interface(DEFAULT_INTERFACE_VALUE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The old version seems better to me.

Copy link
Collaborator

@emildinchev emildinchev left a comment

Choose a reason for hiding this comment

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

Most of the Metadata interfaces simply extends the SubmodelElementAttributes interface without any new definition. Are these new interfaces really needed?

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