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

.equals methods for collection-like elements #50

Open
mjacoby opened this issue Dec 9, 2022 · 4 comments
Open

.equals methods for collection-like elements #50

mjacoby opened this issue Dec 9, 2022 · 4 comments
Assignees
Labels
blocked Awaiting feedback from IDTA or AAS Specs WG so that we can proceed bug Something isn't working V.1.1.0 Final Release V.1.1.0 incl. additional features

Comments

@mjacoby
Copy link
Contributor

mjacoby commented Dec 9, 2022

For some collection-like AAS type classes the equals method does not work as expected, e.g.

Property property1 = new DefaultProperty.Builder()
		.idShort("property1")
		.build();
Property property2 = new DefaultProperty.Builder()
		.idShort("property2")
		.build();
SubmodelElementCollection smc1 = new DefaultSubmodelElementCollection.Builder()
		.value(List.of(property1, property2))
		.build();
SubmodelElementCollection smc2 = new DefaultSubmodelElementCollection.Builder()
		.value(List.of(property2, property1))
		.build();
// expected: true, actual: false
boolean equals = Objects.equals(smc1, smc2);

returns false although SubmodelElementCollection is defined to not be ordered. Same goes for Submodel class.

For SubmodelElementList this issue is more complex as it has the ordered property.

Property property1 = new DefaultProperty.Builder()
		.idShort("property1")
		.build();
Property property2 = new DefaultProperty.Builder()
		.idShort("property2")
		.build();
SubmodelElementList smc1 = new DefaultSubmodelElementList.Builder()
		.value(List.of(property1, property2))
		.build();
SubmodelElementList smc2 = new DefaultSubmodelElementList.Builder()                
		.value(List.of(property2, property1))
		.build();
smc1.setOrderRelevant(false);
smc2.setOrderRelevant(false);
// expected: true, actual: false
boolean equalsWithoutOrder = Objects.equals(smc1, smc2);
smc1.setOrderRelevant(true);
// expected: false, actual: false
boolean equalsWithOrderOneElement = Objects.equals(smc1, smc2);
smc2.setOrderRelevant(true);
// expected: false, actual: false
boolean equalsWithOrder = Objects.equals(smc1, smc2);

All .equals calls return false in this example, whereas equalsWithoutOrder should be true as order should be ignored.

This behavior is not compliant with the current specification.
However, there is a discussion if the specification should be updated to ensure order in such collections (admin-shell-io/aas-specs#248).

Before any release we should make sure that we are compliant to the specification.

@mjacoby mjacoby added the bug Something isn't working label Dec 9, 2022
@mjacoby mjacoby moved this to 🆕 New in Eclipse AAS4J Dev Dec 13, 2022
@twebermartins twebermartins moved this from 🆕 New to 📋 Backlog in Eclipse AAS4J Dev Dec 13, 2022
@twebermartins
Copy link
Contributor

Check the final release spec if this issue has been addressed otherwise discuss internally

@mjacoby
Copy link
Contributor Author

mjacoby commented Jul 4, 2023

@mjacoby to check if this issue is now resolved

@mjacoby
Copy link
Contributor Author

mjacoby commented Jul 7, 2023

Current status

Issue is not resolved.
Corresponding issue in spec discussion (admin-shell-io/aas-specs#248) has been closed but is no concrete answer has been given or given answer contradicts specification. I added a new post asking for clarification.

I also create a corresponding unit test testing the expected behavior according to the spec. It is available on the branch bug/equals https://github.com/eclipse-aas4j/aas4j/blob/bug/equals/model/src/test/java/org/eclipse/digitaltwin/aas4j/v3/model/impl/EqualsTest.java and 7/16 tests are currently failing.

Before updating our implementation we need to clarify the expected behavior with the spec team.

@twebermartins twebermartins added the blocked Awaiting feedback from IDTA or AAS Specs WG so that we can proceed label Jul 18, 2023
@twebermartins twebermartins added the V.1.0.0 Final Release V.1.0.0 following EF processes label Sep 12, 2023
@twebermartins twebermartins added V.1.1.0 Final Release V.1.1.0 incl. additional features and removed V.1.0.0 Final Release V.1.0.0 following EF processes labels Nov 21, 2023
@mhrimaz
Copy link

mhrimaz commented Feb 9, 2024

@mjacoby
Other than ordering issue in equals, there is also problem with custom classes.
Equality of class should be replaced by a more generic logic


can be changed to:
Property.class.isInstance(this)==false|| Property.class.isInstance(obj)==false

Furthermore, the casting to DefaultProperty is too restrictive and unnecessary and can be changed to:

Property other = (Property) obj;
return Objects.equals(this.valueType, other.getValueType()) &&
    Objects.equals(this.value, other.getValue()) &&
    Objects.equals(this.valueId, other.getValueId()) &&
    Objects.equals(this.embeddedDataSpecifications, other.getEmbeddedDataSpecifications()) &&
    Objects.equals(this.category, other.getCategory()) &&
    Objects.equals(this.idShort, other.getIdShort()) &&
    Objects.equals(this.displayName, other.getDisplayName()) &&
    Objects.equals(this.description, other.getDescription()) &&
    Objects.equals(this.extensions, other.getExtensions()) &&
    Objects.equals(this.semanticId, other.getSemanticId()) &&
    Objects.equals(this.supplementalSemanticIds, other.getSupplementalSemanticIds()) &&
    Objects.equals(this.qualifiers, other.getQualifiers());

two questions can be raised here:

  • is null and empty list equal?
  • is null and empty string equal?
    I can help to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Awaiting feedback from IDTA or AAS Specs WG so that we can proceed bug Something isn't working V.1.1.0 Final Release V.1.1.0 incl. additional features
Projects
Status: 📋 Backlog
Development

No branches or pull requests

3 participants