From 7cba137e4a0a9e82e887113c2b676580a663dab7 Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Thu, 2 Jul 2026 19:12:02 +0200 Subject: [PATCH] Skip type-incompatible imports in ModelAssembler.resolveImports resolveImports resolves fragment by elementId via ModelUtils.findElementById, which returns the first match regardless of type, and then eSets it onto the target reference. When the model contains a different-typed element sharing the imported id, the eSet fails with a ClassCastException and aborts assembly of the whole application model. For example ResourceHandlerTest fails in the I-builds with "BindingTableImpl cannot be cast to MBindingContext", because BindingToModelProcessor.createTable() gives each generated BindingTable the binding-context id as its own elementId, so a fragment importing that BindingContext resolves to the same-id BindingTable. Skip such an import with a warning instead: an import that resolves to an incompatible element is a contribution error, not a reason to abort assembly. The reference is left unset and every other fragment still applies. Also skip, rather than NPE, an unresolved import on a many-valued feature. Add a regression test covering the type mismatch. Fixes https://github.com/eclipse-platform/eclipse.platform.ui/issues/4148 --- .../META-INF/MANIFEST.MF | 2 +- .../ui/internal/workbench/ModelAssembler.java | 13 +++++ .../tests/workbench/ModelAssemblerTests.java | 50 +++++++++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/bundles/org.eclipse.e4.ui.workbench/META-INF/MANIFEST.MF b/bundles/org.eclipse.e4.ui.workbench/META-INF/MANIFEST.MF index 79b52fd9e9a..be8da055481 100644 --- a/bundles/org.eclipse.e4.ui.workbench/META-INF/MANIFEST.MF +++ b/bundles/org.eclipse.e4.ui.workbench/META-INF/MANIFEST.MF @@ -1,7 +1,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-SymbolicName: org.eclipse.e4.ui.workbench;singleton:=true -Bundle-Version: 1.18.300.qualifier +Bundle-Version: 1.18.400.qualifier Bundle-Name: %pluginName Bundle-Vendor: %providerName Bundle-Localization: plugin diff --git a/bundles/org.eclipse.e4.ui.workbench/src/org/eclipse/e4/ui/internal/workbench/ModelAssembler.java b/bundles/org.eclipse.e4.ui.workbench/src/org/eclipse/e4/ui/internal/workbench/ModelAssembler.java index 581e1243588..3941fa5af01 100644 --- a/bundles/org.eclipse.e4.ui.workbench/src/org/eclipse/e4/ui/internal/workbench/ModelAssembler.java +++ b/bundles/org.eclipse.e4.ui.workbench/src/org/eclipse/e4/ui/internal/workbench/ModelAssembler.java @@ -775,7 +775,20 @@ public void resolveImports(List imports, List { + if (element != null && !feature.getEType().isInstance(element)) { + // An import that resolves to a different-typed element (e.g. two + // elements sharing an elementId) is a contribution error, not a + // reason to abort assembling the whole application model. + warn("Skipping import for feature {} on {}: resolved element {} is not assignable to the feature type", //$NON-NLS-1$ + feature.getName(), target, element.getElementId()); + return; + } if (feature.isMany()) { + if (element == null) { + warn("Skipping unresolved import for many-valued feature {} on {}", //$NON-NLS-1$ + feature.getName(), target); + return; + } error(""" Replacing in {}. Feature={}. diff --git a/tests/org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/workbench/ModelAssemblerTests.java b/tests/org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/workbench/ModelAssemblerTests.java index 7b658aab90d..c11f5b41c5b 100644 --- a/tests/org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/workbench/ModelAssemblerTests.java +++ b/tests/org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/workbench/ModelAssemblerTests.java @@ -17,6 +17,7 @@ package org.eclipse.e4.ui.tests.workbench; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import jakarta.annotation.PostConstruct; @@ -44,6 +45,7 @@ import org.eclipse.e4.ui.internal.workbench.swt.E4Application; import org.eclipse.e4.ui.model.application.MApplication; import org.eclipse.e4.ui.model.application.MApplicationElement; +import org.eclipse.e4.ui.model.application.commands.MCommand; import org.eclipse.e4.ui.model.application.impl.ApplicationFactoryImpl; import org.eclipse.e4.ui.model.application.ui.MUIElement; import org.eclipse.e4.ui.model.application.ui.advanced.MArea; @@ -340,6 +342,54 @@ public void testImports_noImportElementId() throws Exception { assertEquals("Could not resolve import for null", logMessages.poll()); } + /** + * Tests that an import resolving to an element of an incompatible type (two + * elements sharing an elementId) is skipped with a warning instead of aborting + * model assembly with a {@link ClassCastException}. See + * issue + * 4148. + */ + @Test + public void testImports_typeIncompatibleElement() throws Exception { + List imports = new ArrayList<>(); + List addedElements = new ArrayList<>(); + + final String sharedElementId = "testImports_typeIncompatible_id"; + + // The fragment imports a window with the shared id ... + MTrimmedWindow importWindow = modelService.createModelElement(MTrimmedWindow.class); + importWindow.setElementId(sharedElementId); + MModelFragments fragment = MFragmentFactory.INSTANCE.createModelFragments(); + fragment.getImports().add(importWindow); + imports.add(importWindow); + + // ... but the application only contains a command (a different type) that + // shares that id, so the id-based lookup resolves to a type-incompatible + // element for the MUIElement-typed placeholder reference. + MCommand collidingCommand = modelService.createModelElement(MCommand.class); + collidingCommand.setElementId(sharedElementId); + application.getCommands().add(collidingCommand); + + MPlaceholder placeholder = modelService.createModelElement(MPlaceholder.class); + placeholder.setRef(importWindow); + addedElements.add(placeholder); + + CountDownLatch countDownLatch = new CountDownLatch(1); + this.logListener.countDownLatch = countDownLatch; + + // Must not throw: the incompatible resolution is skipped, the reference is + // left untouched, and every other fragment still applies. + assembler.resolveImports(imports, addedElements); + + assertNotEquals(collidingCommand, placeholder.getRef()); + assertEquals(importWindow, placeholder.getRef()); + + boolean completed = countDownLatch.await(COUNTDOWN_TIMEOUT, TimeUnit.MILLISECONDS); + assertTrue(completed, "Timeout - no event received"); + assertEquals(1, logMessages.size()); + assertTrue(logMessages.poll().startsWith("Skipping import for feature")); + } + /** * Make sure that all fragments and imports are resolved before the * post-processors are run. For reference, see