[starksm64]

"alesj" wrote:

But wouldn't that make Carlo's impl fail on exploded jar?

Yes it would most definitely break on exploded jars.

The issue is that you need to know what the original form of the URL the reference was against. You can't start with a compressed jar with a manifest classpath relative to the jar as a file, and then use the same relative path against the jar as a directory. The jar manifest classpath spec does not deal with this distinction.

If we ever stop unpacking archives such that a jar classpath resolves against an ear which is still packed, we will have the opposite problem of the default relative reference resolution pushing the jar to be outside of the ear.

[jason.greene]

"alesj" wrote:

But wouldn't that make Carlo's impl fail on exploded jar?

Yes it would most definitely break on exploded jars.

The issue is that you need to know what the original form of the URL the reference was against. You can't start with a compressed jar with a manifest classpath relative to the jar as a file, and then use the same relative path against the jar as a directory. The jar manifest classpath spec does not deal with this distinction.

If we ever stop unpacking archives such that a jar classpath resolves against an ear which is still packed, we will have the opposite problem of the default relative reference resolution pushing the jar to be outside of the ear.

So to truly solve this issue, vfs urls would need to be completely logical, and have no distinguishing difference between an exploded directory and a compressed jar. Those really are physical aspects.

[jason.greene]

"alesj" wrote:

But wouldn't that make Carlo's impl fail on exploded jar?

Yes it would most definitely break on exploded jars.

The issue is that you need to know what the original form of the URL the reference was against. You can't start with a compressed jar with a manifest classpath relative to the jar as a file, and then use the same relative path against the jar as a directory. The jar manifest classpath spec does not deal with this distinction.

If we ever stop unpacking archives such that a jar classpath resolves against an ear which is still packed, we will have the opposite problem of the default relative reference resolution pushing the jar to be outside of the ear.

So to truly solve this issue, vfs urls would need to be completely logical, and have no distinguishing difference between an exploded directory and a compressed jar. Those really are physical aspects.

Which seems to indicate that Ales' consistent use of / on non-leaf urls is the correct approach.

[starksm64]

Which seems to indicate that Ales' consistent use of / on non-leaf urls is the correct approach.

It is within a consistent context like only the vfs using the URLs. The problem is taking a relative path out of the context in which it was originally found, and then simply using new URL(jar, path) where the jar path may or may not be a directory in the spec sense. In reality URL(jarParent, path) should always work as jarParent "should" always be a directory. For ears this always works as they have no manifest classpath notion. For wars its confusing because they can have a manifest classpath as well as nested jars.

If we built the classpath based on the original context and propagated it with the file with the manifest we would be ok, but this requires non-jdk api usage.

[adrian.brock]

So to truly solve this issue, vfs urls would need to be completely logical, and have no distinguishing difference between an exploded directory and a compressed jar. Those really are physical aspects.

Which seems to indicate that Ales' consistent use of / on non-leaf urls is the correct approach.

I'm not sure it solves the real problem, but it would certainly provide a consistent basis on which it could be solved.

The problem as currently described by Ales is that adding a "/" helps webapps/libraries
that naturally expect their directories to be unpacked (most webservers do it)
so the directory urls will always end with /

Carlo's code is expecting his persistent units to be packed in jars.

In practice, both are wrong and both should take more care when processing urls.

So we can do it one way or the other in the VFS, the / on the end is more logical
but its bound to break somebody else's code besides jpa (and vice versa).

The fix to Carlo's code is one of:

1) Use the VFS (which will work however Ales changes the URLs)

-URL url = di.getFile("").toURL();
-return new URL(url, jar);
+ VirtualFile di = di.getFile("");
+ return di.getParent().getChild(jar).toURL(); // Obviously needs to check for null

2) Handle the URLs correctly

URL url = di.getFile("").toURL();
+if (url.getPath().endsWith('/')
+ return new URL(url, "../" + jar);
return new URL(url, jar);

[adrian.brock]

So we can do it one way or the other in the VFS, the / on the end is more logical

It still looks weird though to see a packed jar url followed by a /
but it is logical since it does contain children.

[wolfc]

I'll implement the second construct. It leads to regression when deploying output directories in unit tests, but then again relative path is underspecified in those cases.
The spec could use a formal definition of 'a directory containing an exploded jar file'.

[jason.greene]

"wolfc" wrote:
I'll implement the second construct. It leads to regression when deploying output directories in unit tests, but then again relative path is underspecified in those cases.
The spec could use a formal definition of 'a directory containing an exploded jar file'.

Any reason to use it over the first construct? It should be more reliable. Does the above regression occur with it as well?

[wolfc]

Any reason to use it over the first construct? It should be more reliable. Does the above regression occur with it as well?

The first construct doesn't work if the deployment unit base is the VFS root. Then there is no parent.

I could do the following to eliminate the no parent problem:

-URL url = di.getFile("").toURL();
-return new URL(url, jar);
+VirtualFile deploymentUnitFile = di.getFile("");
+VirtualFile parent = deploymentUnitFile.getParent();
+VirtualFile baseDir = (parent != null ? parent : deploymentUnitFile);
+VirtualFile jarFile = baseDir.getChild(jar);
+if(jarFile == null)
+ throw new RuntimeException("could not find child '" + jar + "' on '" + baseDir + "'");
+return jarFile.toURL();

But depending on how you read the specification it could be interpreted as a violation. Because jar files are no longer relative to the physical location, if the VFS is rooted at the deployment unit.
If the formal definition of 'a directory containing an exploded jar' is: a directory with the same name as the jar in question containing the extracted contents of said jar (which is how we define it). Then you've eliminated the output directory use case because you must root at the parent of the '.jar' directory and you're good to go.

[adrian.brock]

"wolfc" wrote:

But depending on how you read the specification it could be interpreted as a violation. Because jar files are no longer relative to the physical location, if the VFS is rooted at the deployment unit.

It's not a violation if you read the JavaEE spec, we're just not required to support it
and therefore it warns it is not portable.

e.g. JavaEE5 8.2.1.2 (talking about manifests)

Top level JAR files that are processed by a deployment tool should not contain
Class-Path entries; such entries would, by definition, reference other files external to the
deployment unit. A deployment tool is not required to process such external
references.

My emphasis.

On your unit tests, you are just constructing your VFS contexts wrongly.
You need to include "external references" in your VFS context by including the parent
within the VFS context you construct.

e.g. we support "external references" if you put them in deploy/
because the deployment file is constructed as

VFS context = VFS.getVFS("deploy");
VirtualFile deployment = context.getRoot().getChild(deploymentName);

so that deployment can see the whole deploy folder.

But a deployment that explicitly lists its URL in the scanner won't have a parent
because there is no equivalent of the "deploy" folder for them.

[adrian.brock]

e.g. we support "external references" if you put them in deploy/
because the deployment file is constructed as

I wouldn't recommend people do it for a different reason in this case,
which is that you would end up deploying the classes twice.
Once as part of scanning deploy/ and once as an external reference in
a different deployment.

[alesj]

"wolfc" wrote:

The first construct doesn't work if the deployment unit base is the VFS root. Then there is no parent.

I could do the following to eliminate the no parent problem:

What about if you just do a simple check?

 VFSDeploymentUnit parent = di.getParent();
 if (parent != null)
 {
 VirtualFile parentRoot = parent.getRoot();
 return parentRoot.findChild(jar).toURL();
 }
 else
 {
 URL url = di.getRoot().toURL();
 if (url.getPath().endsWith("/"))
 {
 return new URL(url, "../" + jar);
 }
 else
 {
 return new URL(url, jar);
 }

 }

[alesj]

"alesj" wrote:

"wolfc" wrote:

The first construct doesn't work if the deployment unit base is the VFS root. Then there is no parent.

I could do the following to eliminate the no parent problem:

What about if you just do a simple check?

I guess this needs more fiddling around
since the term relative in jpa is extremely poorly defined.

[wolfc]

...
My emphasis.

On your unit tests, you are just constructing your VFS contexts wrongly.
You need to include "external references" in your VFS context by including the parent
within the VFS context you construct.

e.g. we support "external references" if you put them in deploy/
because the deployment file is constructed as

VFS context = VFS.getVFS("deploy");
VirtualFile deployment = context.getRoot().getChild(deploymentName);

so that deployment can see the whole deploy folder.

But a deployment that explicitly lists its URL in the scanner won't have a parent
because there is no equivalent of the "deploy" folder for them.

The use case doesn't have any 'weird' manifest entries. It's a maven project with src/test/resources/META-INF/persistence.xml. Now you would want to test this before moving on. But I see your point, the unit test should take 'target' as it's root and 'test-classes' as the deployment unit.
I had lib/common.jar in src/test/resources/lib which is unrealistic, in real life it should come in via the dependency plugin to target/lib.

As for Ales's proposal:
a.ear/b/c/d/persistence.jar with jar reference ../e/common.jar must lead to a.ear/b/c/e/common.jar. So starting from the root is not an option. Without relativity the universe comes to an end.