opensourceBIM / BIMserver

The open source BIMserver platform
GNU Affero General Public License v3.0
1.56k stars 611 forks source link

Multirelations #1335

Open PavelWhiteTiger opened 3 weeks ago

PavelWhiteTiger commented 3 weeks ago

When I add a relationship using my client application, this method may duplicate the relationships. image

PavelWhiteTiger commented 3 weeks ago

This example is in the AddReferenceChange class.

PavelWhiteTiger commented 3 weeks ago

Example of creation and using on my app.

IfcRelationship ifcRel = model.getRichModel().create(model.getClient().getMetaDataManager().getPackageMetaData("ifc4x3").getEClass("IfcRelContainedInSpatialStructure"));  <--- Creating rel

for (String relPropertyName : relPropertyNames) { // relPropertyNames "RelatingStructure", "RelatedElements"
    EReference eAttribute = model.getClient().getMetaDataManager().getPackageMetaData("ifc4x3").getEReference(relType, relPropertyName);
    eAttributes.add(eAttribute);
}
HashMap<EReference, List<IdEObject>> attributeValuesMap = new HashMap<>();
    for (int i = 0; i < eAttributes.size(); i++) {
        attributeValuesMap.put(eAttributes.get(i), listElements.get(i));
}

        for (EReference eReference : attributeValuesMap.keySet()) {
            List<IdEObject> objList = attributeValuesMap.get(eReference);
            if (eReference.isMany()) {
                List list = (List) ifcRel.eGet(eReference);
                for (Object o : objList) {
                    list.add(o);
                }
            } else if (objList.size() > 0) {
                ifcRel.eSet(eReference, objList.get(0));
            }
}
hlg commented 3 weeks ago

In theory, yes. But actually I don't see how, unless you add a reference twice (line 77) - then its inverse will also be added twice (line 92). https://github.com/opensourceBIM/BIMserver/blob/28c557d0782a0eadc3143890c13815a356e7683f/BimServer/src/org/bimserver/changes/AddReferenceChange.java#L76-L97

In your example, "RelatingStructure" is a simple feature, but its inverse "IsDecomposedBy" is an aggregate (isMany). We need to add the inverse to any possibly existing inverses. Imagine

Previously, there was a call to fixInverses in the CommitTransactionDatabaseAction, which would recreate all the inverses. This made sense for whole file checkins, but not for lowlevel changes, where it would lead to a gradual accumulation of duplicate inverses with every commit. It was removed in 252850f and that commit made it into 1.5.185. Which version are you using? Do you have that change?

hlg commented 3 weeks ago

Thinking about it, we actually have the information wether a particular feature allows for duplicates or not (isUnique). Thus we could, similar to the EMF EList implementation, protect the model and its virtual objects from users violating the model constraints. In fact, at the moment, in IFC all inverses are SETs and as such unique, but this is not set in stone. Imagine, for example a polygon with a list of points that allow for repitition (say equal first and last point) and each point can be used in multiple polygons. Then, the respective inverse feature would be an aggregate that allows for duplication (BAG). Also, it would not be trivial to realize this protection. Currently every ignorant client can just get the list for an aggregate feature and add to that.

I am not absolutely sure, but I think in ClientIfcModel for example, we work with EList which protects from duplication unless the feature allows for it.

PavelWhiteTiger commented 3 weeks ago

When I use eGet on client I get specially EList whom knows about rules, but bimserver works another (there is eGet into addReference ), it get simple list without rules. Which version I use I write later... But fixInverse deleted from commit action.