[E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath #2290

HannesWell · 2024-09-17T22:43:48Z

As said in #423 (comment) this is the current state of my stalled work to migrate E4 Xpath off the old and unmaintained apache.commons.jxpath library.
The basic idea is to provide a org.w3c.dom.Element view/wrapper for an EObject so that an javax.xml.xpath.Xpath can operate on it.
As mentioned this is heavily work in progres, not yet functional and a lot has to be cleaned up before this can be used (it contains a lot of try out code).

@ptziegler if you or anybody else like to take this over and complete it please feel free. I also would find this an interesting topic, but I have currently no time to work on this. But if you don't have time either, I might continue this by myself in the future.

I have also extracted some minor improvements that can be applied now already in #2289.

Fixes #423

github-actions · 2024-09-17T23:28:33Z

Test Results

1 815 files - 3 1 815 suites - 3 1h 28m 34s ⏱️ +18s
7 735 tests - 4 7 507 ✅ - 3 228 💤 ±0 0 ❌ - 1
24 366 runs - 12 23 617 ✅ - 11 749 💤 ±0 0 ❌ - 1

Results for commit 063ed05. ± Comparison against base commit ef4c5db.

This pull request removes 4 tests.

org.eclipse.e4.emf.xpath.test.EDynamicPropertyHandlerTest ‑ test_getProperty
org.eclipse.e4.emf.xpath.test.EDynamicPropertyHandlerTest ‑ test_getPropertyNames
org.eclipse.e4.emf.xpath.test.EDynamicPropertyHandlerTest ‑ test_getProperty_When_UnknownProperty_Expect_NullResult
org.eclipse.e4.emf.xpath.test.EDynamicPropertyHandlerTest ‑ test_setProperty

♻️ This comment has been updated with latest results.

ptziegler · 2024-09-23T08:05:46Z

I played around with this draft and I think the initial approach can work, with the main obstacles being:

jxpath has the application as root, while xpath has the document. This leads to weird situation where the XPath "/" has to be translated to "/application/".
The handling of the parent context is something that needs to be extensively tested. Also whether e.g. "/" is now the root of the parent context. I'm also not sure how this would be encoded in XML. I.e. whether both documents need to be merged or whether the "child" application needs to be appended to the "parent" application or if it's something completely different.

How should we proceed here? I don't think I can push directly to your branch. So should I create a separate branch where I do my own development on?

HannesWell · 2024-09-23T21:09:33Z

I played around with this draft and I think the initial approach can work,

Awesome!

with the main obstacles being:

jxpath has the application as root, while xpath has the document. This leads to weird situation where the XPath "/" has to be translated to "/application/".

Couldn't this be fixed by adding a placeholder/virtual/dummy document?

The handling of the parent context is something that needs to be extensively tested. Also whether e.g. "/" is now the root of the parent context. I'm also not sure how this would be encoded in XML. I.e. whether both documents need to be merged or whether the "child" application needs to be appended to the "parent" application or if it's something completely different.

I cannot say much about this a.t.m.

How should we proceed here? I don't think I can push directly to your branch. So should I create a separate branch where I do my own development on?

Yes you have to create your own branch and PR, but you could add a link to this. If you have created it, this can be closed.

laeubi · 2024-09-24T04:34:56Z

I'm also not sure how this would be encoded in XML. I.e. whether both documents need to be merged or whether the "child" application needs to be appended to the "parent" application or if it's something completely different.

Can you explain what exactly is the problem/question?

jxpath has the application as root, while xpath has the document. This leads to weird situation where the XPath "/" has to be translated to "/application/"

I would expect that application is the document element or do I understand the problem wrong?

ptziegler · 2024-09-24T04:55:17Z

Can you explain what exactly is the problem/question?

I'm simply not sure how the parent context is handled in jxpath. But until we can properly ready the current context, this doesn't have a very high priority on my side.

I would expect that application is the document element or do I understand the problem wrong?

Given the following XML document:

<foo>
    <bar/>
    <bar/>
    <bar/>
</foo>

When converted to a Java document, you get the following object structure:

- Document
  - Element (foo)
    - Element (bar)
    - Element (bar)
    - Element (bar)

Evaluating the XPath "/" on any node returns Document and not Element(foo).

laeubi · 2024-09-24T05:07:16Z

If I understand right we already implement the DOM API here (maybe something better placed at EMF directly? @merks ?) so can't Application implement Document + Element here and simplify return it as document and the root element (might be a bit counterintuitive but probably works).

merks · 2024-09-24T05:35:05Z

Stop to ask, why are there so many alternatives to DOM? (Because it's horrible?!)

Goodness knows why folks could not have just use EMF's support for paths?

org.eclipse.emf.ecore.resource.impl.ResourceImpl.getEObject(List)
org.eclipse.emf.ecore.InternalEObject.eObjectForURIFragmentSegment(String)

Probably wasn't pretty enough? Not standard enough? Note powerful enough? Best to hide EMFness?

In any case, no one ever asked me for advice or suggestions, so I have no clue how it was necessary to have the full power of XPath available to reference an object when there are far simpler mechanisms available for doing just that.

I definitely don't want to push this problem down into EMF. People have asked for many things, but never this thing.

laeubi · 2024-09-24T06:14:21Z

Stop to ask, why are there so many alternatives to DOM? (Because it's horrible?!)

EMF is a DOM as well, it just don't implement the (XML) DOM API ;-)

Probably wasn't pretty enough? Not standard enough? Note powerful enough? Best to hide EMFness?

I have no clue but can only assume because the e4 xmi is actually an XML document and XPath is the standard for XML .. anyways Xpath itself do not mandates to use DOM, it supports other (xml) representations as well, thats why I previously mentioned that we probably just need to copy the parser part, because in the end we only need to parse an Xpath Expression and map it to the (EMF) DOM thats what actually is done as of today.

Sadly I have found little to no documentation on this feature so its quite hard to guess what must be supported and how exactly it is mapped or what where the reasons for a design decision. Also the UI for this is really bare....

mickaelistria · 2024-09-24T07:22:03Z

Probably wasn't pretty enough? Not standard enough? Note powerful enough? Best to hide EMFness?

I'm pretty sure it's just because XPath is standard and popular enough to assume most developers will feel comfortable enough with it for this case.
I'm wondering whether the full power of XPath is required here. Looking at all found instances on GitHub https://github.com/search?q=path%253A*.e4xmi+xpath&type=code&ref=advsearch , we can see only a few basic patterns: xpath:/ (root, 95% of occurrences), xpath://mainMenu (select all), xpath:/mainMenu/child[1], xpath://*[@elementId='fragment.contributedMenu1' or @elementId='fragment.contributedMenu2'] (select by attribute). We could consider just sticking to supporting that list.

If EMF already support well an XPath-like syntax to select node and this syntax is xpath enough to expect most users wouldn't need to change their extensions to get the same node selected, we could consider just dropping XPath and adopting the EMF way.
If the is another close but not directly complatible syntax supported natively in EMF, we could consider converting XPath to this syntax.
@merks What do you think is possible/best here to rely on more native EMF features?

merks · 2024-09-24T07:50:55Z

The XPath library being used has the benefit that it operates on any DOM-like structure. The built-in XPath support works only on org.w3c.dom. That's simply nasty such that one must try to serialize the model to a DOM and keep a mapping to work your way back. I haven't looked at the details of prototype. It's not clear to me that cloning jxpath and deleting the unused content would not be the easier approach. Either way, there is a whole whack of complex crap that needs to be maintained...

I think at this point, we are stuck needing to support XPath expressions exactly as they are current used, so we must parse them and evaluate them somehow. Alternative approaches are water under the bridge that can't be pushed back upstream. (I bring it up merely because I do not want EMF, i.e., me personally, to burdened with this, but I'm happy to help the Platform wherever I can.)

ptziegler · 2024-09-24T08:40:42Z

It's not clear to me that cloning jxpath and deleting the unused content would not be the easier approach. Either way, there is a whole whack of complex crap that needs to be maintained...

I believe both approaches are feasible but at least in the long term, we should try to remove the reference JXPath. But given that this will take quite a lot of effort, it also makes sense to simply fork the JXPath project until then.
Perhaps we also come to the conclusion that the XPath approach doesn't work out as well as we had hoped, in which case we still have the alternative to fall back on. I'll try to draft a PR for the fork, separately from the PR for using XPath.

merks · 2024-09-24T09:16:49Z

FYI, in Orbit I build axis1 (horrible but BIRT uses it) from source and publish it to repo.eclipse.org so that we can use BND to create an OSGi build from it as if it were published to Maven central:

https://github.com/eclipse-orbit/orbit-simrel/blob/main/maven-deploy/MavenAxis.jenkinsfile

We could do that with jxpath, or a fork of jxpath, perhaps a fork where only the "CVE" functionality is disabled so that there really isn't much to maintain at all, and it could be rebased on newer versions of jxpath in the future.

Just a thought...

ptziegler · 2024-09-24T09:27:13Z

We could do that with jxpath, or a fork of jxpath, perhaps a fork where only the "CVE" functionality is disabled so that there really isn't much to maintain at all, and it could be rebased on newer versions of jxpath in the future.

That's effectively the case with org.apache.commons.jxpath v1.3.0.v200911051830 that was used previously. Because this plugin doesn't import e.g. the javax.servlet packages, all of the "remote execution" CVEs are effectively irrelevant, as the application would already fail with an exception, when trying to initialize the servlets.

laeubi · 2024-09-24T09:29:59Z

Because this plugin doesn't import e.g. the javax.servlet packages, all of the "remote execution" CVEs are effectively irrelevant, as the application would already fail with an exception, when trying to initialize the servlets.

As jxpath does not run inside a servelt / EE Container, they are effectively irrelevant for where we use that as well ... ;-)

In any case, embedding the code seem more suitable than building something that is similar but named the same as an official artifact.

HannesWell · 2024-10-08T16:39:41Z

Today I stumbled upon the jaxen library, which says:

The Jaxen XPath Engine for Java
[...]
It is also possible to write adapters that treat non-XML trees such as compiled Java byte code
or Java beans as XML, thus enabling you to query these trees with XPath too.

It sounds like this maybe could be an alternative for jXpath. It isn't very active either but it's latest release is only two years old.
And the good thing is, it's already in simrel-Orbit and seems to have no extra dependencies https://github.com/eclipse-orbit/orbit-simrel/blob/75cc23701b2417f530efd3ce51763aef09c5a206/maven-osgi/tp/Maven.target#L678-L680

ptziegler · 2024-10-08T17:46:31Z

Today I stumbled upon the jaxen library, which says:
The Jaxen XPath Engine for Java
[...]
It is also possible to write adapters that treat non-XML trees such as compiled Java byte code
or Java beans as XML, thus enabling you to query these trees with XPath too.
It sounds like this maybe could be an alternative for jXpath. It isn't very active either but it's latest release is only two years old. And the good thing is, it's already in simrel-Orbit and seems to have no extra dependencies https://github.com/eclipse-orbit/orbit-simrel/blob/75cc23701b2417f530efd3ce51763aef09c5a206/maven-osgi/tp/Maven.target#L678-L680

I gave it a quick try, but I don't believe it works as well as it should... For example, you can't "skip" nodes, so expressions like "children/mainMenu" work, but "//mainMenu" doesn't. Getting the current object via "/" also doesn't work...

akurtakov · 2025-01-14T05:47:40Z

Is there progress here? It would be nice to get rid of jxpath and it's CVEs this cycle.

eclipse-platform-bot · 2025-01-14T05:51:53Z

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

bundles/org.eclipse.e4.emf.xpath/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

From b6e9b024238621c74b3996faaac3f3e1d438e66d Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Tue, 14 Jan 2025 05:51:29 +0000
Subject: [PATCH] Version bump(s) for 4.35 stream


diff --git a/bundles/org.eclipse.e4.emf.xpath/META-INF/MANIFEST.MF b/bundles/org.eclipse.e4.emf.xpath/META-INF/MANIFEST.MF
index 74cc46b704..0fd771831e 100644
--- a/bundles/org.eclipse.e4.emf.xpath/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.e4.emf.xpath/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %Bundle-Name
 Bundle-SymbolicName: org.eclipse.e4.emf.xpath
-Bundle-Version: 0.5.0.qualifier
+Bundle-Version: 0.5.100.qualifier
 Bundle-RequiredExecutionEnvironment: JavaSE-17
 Require-Bundle: org.eclipse.emf.ecore;bundle-version="2.35.0",
  org.eclipse.core.runtime;bundle-version="3.29.0"
-- 
2.47.1

Further information are available in Common Build Issues - Missing version increments.

ptziegler · 2025-01-14T08:47:01Z

Is there progress here? It would be nice to get rid of jxpath and it's CVEs this cycle.

Sadly not, at least from my side. A lot of stuff has been piling up over the past months and I fear I won't be able to work on this anytime soon. 😦

laeubi · 2025-01-14T09:51:43Z

@akurtakov @HannesWell @ptziegler I have now just taken the opportunity to try the path to embed jxpath here:

Embedd jxpath #2691

Fixes eclipse-platform#423 Co-authored-by: Patrick Ziegler <[email protected]>

ptziegler · 2025-01-18T19:51:43Z

...lipse.e4.emf.xpath.test/src/org/eclipse/e4/emf/xpath/test/ExampleQueriesApplicationTest.java

+		assertThat(menuEntries).isInstanceOf(MMenu.class);
+		assertEquals("File", ((MMenu) menuEntries).getLabel());


This check fails on the master:

java.lang.AssertionError: Expecting actual: [file=org.eclipse.e4.ui.model.application.ui.menu.impl.MenuImpl@7a729f84 (tags: null, contributorURI: null) (widget: null, renderer: null, toBeRendered: true, onTop: false, visible: true, containerData: null, accessibilityPhrase: null) (label: File, iconURI: null, tooltip: null, mnemonics: null) (enabled: true), help=org.eclipse.e4.ui.model.application.ui.menu.impl.MenuImpl@1dba4e06 (tags: null, contributorURI: null) (widget: null, renderer: null, toBeRendered: true, onTop: false, visible: true, containerData: null, accessibilityPhrase: null) (label: Help, iconURI: null, tooltip: null, mnemonics: null) (enabled: true)] to be an instance of: org.eclipse.e4.ui.model.application.ui.menu.MMenu but was instance of: anonymous class at org.eclipse.e4.emf.xpath.test.ExampleQueriesApplicationTest.testAccessingAllMenus(ExampleQueriesApplicationTest.java:111) at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) at java.base/java.lang.reflect.Method.invoke(Method.java:580) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306) at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63) at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329) at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293) at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306) at org.junit.runners.ParentRunner.run(ParentRunner.java:413) at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:93) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:40) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:520) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:748) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:443) at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.main(RemotePluginTestRunner.java:83) at org.eclipse.pde.internal.junit.runtime.CoreTestApplication.start(CoreTestApplication.java:28) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:208) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:143) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:109) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:439) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:271) at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) at java.base/java.lang.reflect.Method.invoke(Method.java:580) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:668) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:605) at org.eclipse.equinox.launcher.Main.run(Main.java:1481) at org.eclipse.equinox.launcher.Main.main(Main.java:1454)

I assume JXPath is using a different classloader?

I assume JXPath is using a different classloader?

The problem is that in this case jxpath returns the list of children from the ecore mode, which is an anonymous class.
We'll have to add more tests to check the different behavior when an expected type is given and the iterate/stream() method is used vs. this getValue() method without any requirement, for collection results.

ptziegler · 2025-01-18T20:02:21Z

...pse.e4.emf.xpath/src/org/eclipse/e4/emf/xpath/internal/java/JavaXPathContextFactoryImpl.java

+			EObjectContext parent = ((EObjectContext) parentContext);
+			Element rootElement = parent.object2domProxy.get(contextBean);
+			if (rootElement == null) {
+				throw new IllegalArgumentException("Context bean is not from the same tree its parent context");


This is perfectly valid in JXPath, so this must not throw an exception. More generally, I think we both have a general misunderstanding on how the parent context translates to the XML structure.

Documentation is really sparse, at best. But as far as I understand it, the parent context is only used to share the configuration. Meaning the locale, variables etc.
But this doesn't mean that the XML document of the child context is contained by the XML document of the parent context.

For example, this is the test case I had extended for my implementation:

/******************************************************************************* * Copyright (c) 2018, 2025 vogella GmbH and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 * which accompanies this distribution, and is available at * https://www.eclipse.org/legal/epl-2.0/ * * SPDX-License-Identifier: EPL-2.0 * * Contributors: * Lars Vogel <[email protected]> - initial contribution ******************************************************************************/ package org.eclipse.e4.emf.xpath.test; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import org.eclipse.e4.emf.xpath.EcoreXPathContextFactory; import org.eclipse.e4.emf.xpath.XPathContext; import org.eclipse.e4.emf.xpath.XPathContextFactory; import org.eclipse.e4.ui.internal.workbench.E4XMIResourceFactory; import org.eclipse.e4.ui.model.application.MAddon; import org.eclipse.e4.ui.model.application.MApplication; import org.eclipse.e4.ui.model.application.commands.impl.CommandsPackageImpl; import org.eclipse.e4.ui.model.application.impl.ApplicationPackageImpl; import org.eclipse.e4.ui.model.application.ui.advanced.impl.AdvancedPackageImpl; import org.eclipse.e4.ui.model.application.ui.basic.impl.BasicPackageImpl; import org.eclipse.e4.ui.model.application.ui.impl.UiPackageImpl; import org.eclipse.e4.ui.model.application.ui.menu.MMenu; import org.eclipse.e4.ui.model.application.ui.menu.impl.MenuPackageImpl; import org.eclipse.e4.ui.model.fragment.MModelFragments; import org.eclipse.e4.ui.model.fragment.MStringModelFragment; import org.eclipse.emf.common.util.URI; import org.eclipse.emf.ecore.EObject; import org.eclipse.emf.ecore.resource.Resource; import org.eclipse.emf.ecore.resource.ResourceSet; import org.eclipse.emf.ecore.resource.impl.ResourceSetImpl; import org.junit.After; import org.junit.Before; import org.junit.Test; public class ExampleQueriesApplicationTest { private ResourceSet resourceSet; private XPathContext xpathContext; private XPathContext xpathChildContext; private Resource resource; private Resource childResource; @SuppressWarnings("restriction") @Before public void setUp() { resourceSet = new ResourceSetImpl(); resourceSet.getResourceFactoryRegistry().getExtensionToFactoryMap() .put(Resource.Factory.Registry.DEFAULT_EXTENSION, new E4XMIResourceFactory()); resourceSet.getPackageRegistry().put(ApplicationPackageImpl.eNS_URI, ApplicationPackageImpl.eINSTANCE); resourceSet.getPackageRegistry().put(CommandsPackageImpl.eNS_URI, CommandsPackageImpl.eINSTANCE); resourceSet.getPackageRegistry().put(UiPackageImpl.eNS_URI, UiPackageImpl.eINSTANCE); resourceSet.getPackageRegistry().put(MenuPackageImpl.eNS_URI, MenuPackageImpl.eINSTANCE); resourceSet.getPackageRegistry().put(BasicPackageImpl.eNS_URI, BasicPackageImpl.eINSTANCE); resourceSet.getPackageRegistry().put(AdvancedPackageImpl.eNS_URI, AdvancedPackageImpl.eINSTANCE); resourceSet.getPackageRegistry().put( org.eclipse.e4.ui.model.application.descriptor.basic.impl.BasicPackageImpl.eNS_URI, org.eclipse.e4.ui.model.application.descriptor.basic.impl.BasicPackageImpl.eINSTANCE); URI uri = URI.createPlatformPluginURI("/org.eclipse.e4.emf.xpath.test/model/Application.e4xmi", true); resource = resourceSet.getResource(uri, true); XPathContextFactory<EObject> f = EcoreXPathContextFactory.newInstance(); xpathContext = f.newContext(resource.getContents().get(0)); URI childUri = URI.createPlatformPluginURI("/org.eclipse.e4.emf.xpath.test/model/fragment.e4xmi", true); childResource = resourceSet.getResource(childUri, true); xpathChildContext = f.newContext(xpathContext, childResource.getContents().get(0)); } @After public void tearDown() { xpathContext = null; resource.unload(); resourceSet.getResources().remove(resource); xpathChildContext = null; childResource.unload(); resourceSet.getResources().remove(childResource); } @Test public void testAccessingTheApplication() { Object application = xpathContext.getValue("/"); assertThat(application).isInstanceOf(MApplication.class); } @Test public void testAccessingTheMainMenu() { Object menu = xpathContext.getValue("//mainMenu"); assertThat(menu).isInstanceOf(MMenu.class); MMenu mMenu = xpathContext.getValue("//mainMenu", MMenu.class); assertNotNull(mMenu); Object menu2 = xpathContext.getValue("/children/mainMenu"); assertThat(menu2).isInstanceOf(MMenu.class); } @Test public void testAccessingAllMenus() { Object menuEntries = xpathContext.getValue("//mainMenu/children"); assertNotNull(menuEntries); assertThat(menuEntries).isInstanceOf(MMenu.class); assertEquals("File", ((MMenu) menuEntries).getLabel()); } @Test public void testAccessingTheModelFragments() { Object modelFragments = xpathChildContext.getValue("/"); assertThat(modelFragments).isInstanceOf(MModelFragments.class); } @Test public void testAccessingTheStringModelFragment() { Object modelFragment = xpathChildContext.getValue("//fragments[1]"); assertThat(modelFragment).isInstanceOf(MStringModelFragment.class); MStringModelFragment mModelFragment = xpathChildContext.getValue("//fragments[1]", MStringModelFragment.class); assertNotNull(mModelFragment); Object modelFragment2 = xpathChildContext.getValue("/fragments[1]"); assertThat(modelFragment2).isInstanceOf(MStringModelFragment.class); } @Test public void testAccessingTheAddons() { Object addon = xpathChildContext.getValue("//elements[1]"); assertThat(addon).isInstanceOf(MAddon.class); MAddon mAddon = xpathChildContext.getValue("//elements[1]", MAddon.class); assertNotNull(mAddon); } }

with the fragment.e4xmi:

<?xml version="1.0" encoding="ASCII"?> <fragment:ModelFragments xmi:version="2.0" xmlns:xmi="http://www.omg.org/XMI" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:application="http://www.eclipse.org/ui/2010/UIModel/application" xmlns:fragment="http://www.eclipse.org/ui/2010/UIModel/fragment" xmi:id="_G1t40NXTEe-i_c0uGwOUng"> <fragments xsi:type="fragment:StringModelFragment" xmi:id="_H8jlENXTEe-i_c0uGwOUng" featurename="addons" parentElementId="xpath:/"> <elements xsi:type="application:Addon" xmi:id="_Je0CANXTEe-i_c0uGwOUng" elementId="org.eclipse.e4.emf.xpath.test.addon.0"/> </fragments> </fragment:ModelFragments>

Note how xpathChildContext.getValue("/") returns an instance of MModelFragments, even though it has the application context as its parent?

This is perfectly valid in JXPath, so this must not throw an exception.

I found another existing use-case were I got that impression as well. The reason why this is exists, because of this test-case:

eclipse.platform.ui/tests/org.eclipse.e4.emf.xpath.test/src/org/eclipse/e4/emf/xpath/test/ExampleQueriesTestCase.java

Line 84 in ef4c5db

assertThrows(JXPathNotFoundException.class, () -> xpathContext.getValue(".[@id='nixda']"));

But probably an exception is only thrown in a more specific case and not generally if the match result is empty.

More generally, I think we both have a general misunderstanding on how the parent context translates to the XML structure.

Documentation is really sparse, at best. But as far as I understand it, the parent context is only used to share the configuration. Meaning the locale, variables etc. But this doesn't mean that the XML document of the child context is contained by the XML document of the parent context.

I fully agree on the first part and that's for example why the XPath object is shared in the current state of this PR, but I'm not sure about the latter. If parent-context is shared but not the XML document that coupling seems very loose. I cannot really tell if that generality is needed.
But yes the documentation is indeed really sparse... I'll check the issue in whose context this was added, maybe that provides more info:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=324954

In general I wonder if the parent context capability is even used, respectively if this E4 Xpath is used through it's java API at all or only in E4-model(fragments)? Because AFAICT the latter never uses a parent context.
If possible and for simplicity we could remove the parent-context handling too?

For example, this is the test case I had extended for my implementation:

Please contribute this extended test in a separate PR, we really need more tests on this topic. So if you have more cases you know that should be tested please add them as well. My problem is that I have almost no real-life use-case I can use to test this change.

HannesWell mentioned this pull request Sep 17, 2024

Remove dependencies on org.apache.commons.jxpath #423

Open

HannesWell force-pushed the javax.xpath branch from 3e0d31e to a8e0384 Compare September 23, 2024 20:26

ptziegler mentioned this pull request Oct 11, 2024

[WIP] [E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath #2387

Draft

akurtakov force-pushed the javax.xpath branch from a8e0384 to c8fea16 Compare January 14, 2025 05:47

HannesWell mentioned this pull request Jan 14, 2025

Embedd jxpath #2691

Open

HannesWell force-pushed the javax.xpath branch from 443239a to bdf11d8 Compare January 17, 2025 23:13

HannesWell changed the title ~~[WIP] [E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath~~ [E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath Jan 18, 2025

HannesWell and others added 2 commits January 18, 2025 20:15

[E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath

b7ca926

Fixes eclipse-platform#423 Co-authored-by: Patrick Ziegler <[email protected]>

Initial support for parent context-handling

063ed05

HannesWell force-pushed the javax.xpath branch from bdf11d8 to 063ed05 Compare January 18, 2025 19:15

ptziegler reviewed Jan 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath #2290

[E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath #2290

HannesWell commented Sep 17, 2024 •

edited

Loading

github-actions bot commented Sep 17, 2024 •

edited

Loading

ptziegler commented Sep 23, 2024

HannesWell commented Sep 23, 2024

laeubi commented Sep 24, 2024

ptziegler commented Sep 24, 2024

laeubi commented Sep 24, 2024

merks commented Sep 24, 2024

laeubi commented Sep 24, 2024

mickaelistria commented Sep 24, 2024

merks commented Sep 24, 2024

ptziegler commented Sep 24, 2024

merks commented Sep 24, 2024

ptziegler commented Sep 24, 2024

laeubi commented Sep 24, 2024 •

edited

Loading

HannesWell commented Oct 8, 2024

ptziegler commented Oct 8, 2024 •

edited

Loading

akurtakov commented Jan 14, 2025

eclipse-platform-bot commented Jan 14, 2025

ptziegler commented Jan 14, 2025

laeubi commented Jan 14, 2025 •

edited

Loading

ptziegler Jan 18, 2025

HannesWell Jan 19, 2025

ptziegler Jan 18, 2025

HannesWell Jan 19, 2025

		assertThat(menuEntries).isInstanceOf(MMenu.class);
		assertEquals("File", ((MMenu) menuEntries).getLabel());

[E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath #2290

Are you sure you want to change the base?

[E4 Xpath] Replace apache.commons.jxpath by javax.xml.xpath #2290

Conversation

HannesWell commented Sep 17, 2024 • edited Loading

github-actions bot commented Sep 17, 2024 • edited Loading

Test Results

ptziegler commented Sep 23, 2024

HannesWell commented Sep 23, 2024

laeubi commented Sep 24, 2024

ptziegler commented Sep 24, 2024

laeubi commented Sep 24, 2024

merks commented Sep 24, 2024

laeubi commented Sep 24, 2024

mickaelistria commented Sep 24, 2024

merks commented Sep 24, 2024

ptziegler commented Sep 24, 2024

merks commented Sep 24, 2024

ptziegler commented Sep 24, 2024

laeubi commented Sep 24, 2024 • edited Loading

HannesWell commented Oct 8, 2024

ptziegler commented Oct 8, 2024 • edited Loading

akurtakov commented Jan 14, 2025

eclipse-platform-bot commented Jan 14, 2025

ptziegler commented Jan 14, 2025

laeubi commented Jan 14, 2025 • edited Loading

ptziegler Jan 18, 2025

Choose a reason for hiding this comment

HannesWell Jan 19, 2025

Choose a reason for hiding this comment

ptziegler Jan 18, 2025

Choose a reason for hiding this comment

HannesWell Jan 19, 2025

Choose a reason for hiding this comment

HannesWell commented Sep 17, 2024 •

edited

Loading

github-actions bot commented Sep 17, 2024 •

edited

Loading

laeubi commented Sep 24, 2024 •

edited

Loading

ptziegler commented Oct 8, 2024 •

edited

Loading

laeubi commented Jan 14, 2025 •

edited

Loading