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

ClassLoader Issues with ObjectNode #30

Closed
matthewjmason opened this issue Jan 2, 2013 · 16 comments
Closed

ClassLoader Issues with ObjectNode #30

matthewjmason opened this issue Jan 2, 2013 · 16 comments
Labels

Comments

@matthewjmason
Copy link

I see that you have overriden com.fasterxml.jackson.databind.node.ObjectNode in your jar. This is causing class loading issues for when trying to invoke your code. When I have both json-schema-validator and jackson-databind on my classpath in eclipse, it is loading the jackson-databind's version of ObjectNode instead of yours. This is causing the following error:

java.lang.NoSuchMethodError: com.fasterxml.jackson.databind.node.ObjectNode.asMap()Ljava/util/Map; at org.eel.kitchen.jsonschema.validator.ObjectValidator.<init>(ObjectValidator.java:68) at org.eel.kitchen.jsonschema.validator.InstanceValidator.validate(InstanceValidator.java:89) at org.eel.kitchen.jsonschema.main.JsonSchema.validate(JsonSchema.java:71) at com.dibs.json_validator.AppTest.testJSON(AppTest.java:58) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:601) at junit.framework.TestCase.runTest(TestCase.java:154) at junit.framework.TestCase.runBare(TestCase.java:127) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:118) at junit.framework.TestSuite.runTest(TestSuite.java:208) at junit.framework.TestSuite.run(TestSuite.java:203) at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)

Is there some standard workaround or solution for this? I would prefer not to mess with my classloader. I am using the latest version of the 1.4 branch.

@fge
Copy link
Collaborator

fge commented Jan 2, 2013

Oh f*... I didn't foresee this problem :/

OK, I'll explain why I did it:

  • Jackson's base implementation doesn't make the underlying container (the Map) final -- I do (and this provides non negligible gains in performance);
  • I added this .asMap() method as a convenience, in order to retrieve a Map<String, JsonNode> view of members in the object;
  • unfortunately, JsonNode has .with() and .withArray() methods which return respectively an ObjectNode and ArrayNode, which are subclasses of JsonNode :(

So, this is clearly a bug, and I have never seen it here (I am using IDEA, but that probably does not explain things). The solution, of course, is to remove that .asMap() method from "my" implementation of ObjectNode.

Would you have the possibility to alter your setup to use a fork of master instead? The commit is not there yet, but I'll tell you when it is there. Also, out of curiosity, what version of Jackson is loaded?

Sorry for that :/

@fge
Copy link
Collaborator

fge commented Jan 2, 2013

OK, the commit is now in master. All tests still pass etc. I am ready to push out a new 1.4.x version which includes this commit, but would you mind testing master before I do that, if at all possible? Iterating through maven releases can prove painful...

@fge
Copy link
Collaborator

fge commented Jan 2, 2013

Sorry, git mishap... Can you pull again? I forgot to include the modification to ObjectNode proper :(

fge added a commit that referenced this issue Jan 2, 2013
Issue #30 shows that this can lead to a problem I would never had foreseen until
I saw it with my own eyes: the fact that the new ObjectNode implementation adds
an .asMap() method can cause problems.

As a result, re-introduce a JacksonUtils class with a sole asMap() method which
does the same thing.

Jackson is to blame here: ContainerNode<T> returns subclasses of itself in only
two methods. Two. But that is enough so that I must override ObjectNode and
ArrayNode this way, with this kind of problems lurking behind :/

Link: #30
Signed-off-by: Francis Galiegue <[email protected]>
@fge
Copy link
Collaborator

fge commented Jan 2, 2013

Yet another mishap... Sorry about this :/

@matthewjmason
Copy link
Author

I pulled and tested on master and no longer encounter the error. I can stay on master, I had only downgraded to 1.4 to troubleshoot.

While there is no longer a conflict of method signatures, it would seem to me that there is still a latent issue with classloading since there are two copies of this class. While I don't see any problematic behavior now, I wouldn't be surprised if this manifests itself later. I have previously run into ClassNotFoundExceptions if I have a duplicate class on the classpath. Is there a reason you can't just extend ObjectNode and override to add your desired functionality?

@fge
Copy link
Collaborator

fge commented Jan 2, 2013

Is there a reason you can't just extend ObjectNode and override to add your desired functionality?

Apart from JsonNode's .with() and .withArray() methods, I also use a custom JsonNodeFactory to account for numeric node equality.

I figured out that just replacing ObjectNode and ArrayNode was the safest way around, and it looks like it was until you opened both this bug and my eyes on another problem: ObjectNode may have been loaded before and the benefits of my implementation thrown out the window...

So, you are right, I need to extend it. I just need to figure out how I'll combine the whole so that .equals() and .hashCode work as fast as they currently work, and still obey their respective contracts... All the while dealing with JsonNode's misdesign.

In the meanwhile, the current solution is easily backported to 1.4.x, do you have any interest in my doing so? It is a critical enough bug that I spawn a new 1.4.x with just that commit in any case.

@matthewjmason
Copy link
Author

I have written my schemas based around the draft version 4. I know this is not finalized yet, but I had an easier time accomplishing things with it. My schemas are working with both 1.4.X and 1.5. I am open to using whichever branch you feel is the best bet for an enterprise environment.

@fge
Copy link
Collaborator

fge commented Jan 2, 2013

OK well, in an enterprise environment, I guess the best bet is the currently declared stable version, right?

In this case, README.md clearly states that you should use 1.4.x, since 1.5.x is the development version. You have clearly uncovered a bug which affects both versions, and which is backportable to 1.4.x. My recommendation is therefore to wait until 1.4.2 is published.

You have just made me wonder something: you say you are using draft v4, how do you declare using draft v4 using the current code? Is that using $schema? This is one part of the API which I plan to redesign/extend for 1.5.x and upper since it is clearly not satisfactory to my eyes. Also, do you define your own keywords/format attributes?

fge added a commit that referenced this issue Jan 2, 2013
Issue #30 shows that this can lead to a problem I would never had foreseen until
I saw it with my own eyes: the fact that the new ObjectNode implementation adds
an .asMap() method can cause problems.

As a result, re-introduce a JacksonUtils class with a sole asMap() method which
does the same thing.

Jackson is to blame here: ContainerNode<T> returns subclasses of itself in only
two methods. Two. But that is enough so that I must override ObjectNode and
ArrayNode this way, with this kind of problems lurking behind :/

Link: #30
Signed-off-by: Francis Galiegue <[email protected]>
fge added a commit that referenced this issue Jan 2, 2013
* Do not add .asMap() method to custom (package-equal) ObjectNode
  implementation: if another implementation is loaded before ours, this will
  lead to class loader errors (issue #30)

Francis Galiegue (1):
      Remove .asMap() method from custom ObjectNode implementations

Signed-off-by: Francis Galiegue <[email protected]>
@fge
Copy link
Collaborator

fge commented Jan 2, 2013

OK, I have completed maven artifact publication for 1.4.2, which fixes this bug in particular (although the underlying problem is still there as you have clearly demonstrated).

In short, if you are using 1.4.2:

  • you will be using the first ObjectNode implementation which your classpath chooses to load: either the one from jackson-databind or the custom, bundled implementation;
  • while the core problem is not solved yet (that is, I should really define an implementation which inherits ObjectNode even if this means scrapping out its internals), the error you have seen should not crop up;
  • more importantly, .equals() and .hashCode() will behave correctly for whichever implementation is loaded first.

Unfortunately, I cannot guarantee the delay for availability of maven artifacts... I'd appreciate if you could update this bug when you see 1.4.2 available.

fge added a commit that referenced this issue Jan 3, 2013
And make our CustomJsonNodeFactory return these overrides instead of the plain
ObjectNode and ArrayNode.

See issue #30.

Signed-off-by: Francis Galiegue <[email protected]>
@fge
Copy link
Collaborator

fge commented Jan 5, 2013

OK, 1.4.2 is here with a quick fix and a more complete fix is in master.

Do you want me to backport the more complete fix to 1.4.x?

@matthewjmason
Copy link
Author

Yes, if you could port the complete fix to 1.4.x it would be very helpful.

I believe you asked for my usage of schemas in an earlier post. Here is what I have done so far:

{ "$schema": "http://json-schema.org/draft-04/schema#", "title": "homepage", "description": "JSON representation of a homepage", "type": "object", "properties": { "type": { "type": "string", "enum": ["homepage"] }, "note": { "type": "string" }, "status": { "type": "string" }, "modules": { "type": "array", "items": { "allOf": [ { "$ref": "http://XXXX/module.json" } ] } } }, "additionalProperties": false }

{ "$schema": "http://json-schema.org/draft-04/schema#", "id": "module", "title": "module", "type": "object", "description": "Modules specfication", "properties": { "visible": { "type": "string", "enum": ["true", "false"] }, "type": { "type": "string", "enum": ["hero", "promo", "locationsList", "collectionsList", "featured", "collections"] }, "noToggle": { "type": "string", "enum": ["true", "false"] }, "index": { "type": "string" }, "title": { "type": "string" }, "items": { "type": "array", "items": { "allOf": [ { "$ref": "http://XXXX/item.json" } ] } } }, "additionalProperties": false }

{ "$schema": "http://json-schema.org/draft-04/schema#", "id": "item", "title": "item", "type": "object", "description": "Items specification", "properties": { "type": { "type": "string", "enum": ["product", "promo", "head", "primary"] }, "visible": { "type": "string", "enum": ["true", "false"] }, "position": { "type": "string" }, "dibs_L_id": { "type": "string" }, "title": { "type": "string" }, "title_color": { "type": "string" }, "link_text": { "type": "string" }, "link_url": { "type": "string" }, "dealer_logo_url": { "type": "string" }, "dealer_title": { "type": "string" }, "image_url": { "type": "string" }, "image_width": { "type": "string" }, "image_height": { "type": "string" }, "items": { "type": "array", "items": { "allOf": [ { "$ref": "#" } ] } } }, "additionalProperties": false }

@fge
Copy link
Collaborator

fge commented Jan 7, 2013

OK, I will backport the fix soon.

Just one note about your schemas; you have a lot of:

{
    "type": "string":
    "enum": [ "some", "string", "values", "here" ]
}

The type is actually unneeded in this situation: enum does a complete equality on each of its arguments.

Also, you use, in one place:

{
    "allOf": [
        { "$ref": "whatever" }
    ]
}

No need for allOf here, juse use { "$ref": "whatever" }. Anyway, those are details.

I'll let you know when I have pushed 1.4.3.

fge added a commit that referenced this issue Jan 7, 2013
And make our CustomJsonNodeFactory return these overrides instead of the plain
ObjectNode and ArrayNode.

Update Javadoc as well.

See issue #30.

Signed-off-by: Francis Galiegue <[email protected]>
@fge
Copy link
Collaborator

fge commented Jan 7, 2013

1.4.3 has been released. When you have the opportunity, can you test it and report?

@matthewjmason
Copy link
Author

I pulled the 1.4.X branch -- the pom says 1.4.4 -- and it passes my (limited) test case. Thanks a lot.

By the way, is there a reason you release versions with a version of X.Y.Z-SNAPSHOT instead of X.Y.Z-RELEASE? It would seem appropriate to make the distinction that it was a release version. I bring this up because our Nexus repository policy is to only allow third-party libs that are a release version. For now I am manually changing this in your pom but it seems like you would want formal releases to be labeled as such.

Thanks again,
Matt

@fge fge reopened this Jan 8, 2013
@fge
Copy link
Collaborator

fge commented Jan 8, 2013

Wait wait wait...

the pom says 1.4.4

and

is there a reason you release versions with a version of X.Y.Z-SNAPSHOT instead of X.Y.Z-RELEASE?

There is none, apart from my scarce knowledge of maven and, maybe (I cannot tell), the fact that I use SonaType's release plugin. To be brutally honest, I know it works, but I don't know why (or how) it works.

All I know is that this plugin modifies pom.xml twice, in two different commits:

  • one to set the release to version x and publish that (this is what is available on maven),
  • one to set the release to version x+1.

But as I do NOT want to pollute my repository with commits NOT generated by me (I positively hate this plugin's commit messages, especially since it leaves no opportunity to edit them, and I hate that a plugin creates a commit for me), I just let these commits go to a "throw out" branch and publish to github a rebased version of changes with the shortlog etc.

In effect, it means that while code-wise, what is published via Sonatype is equivalent to what is on the matching tag on GitHub, what is on GitHub is what I want to see in the tree, "freed of" Sonatype's commit garbage.

(another reason why I do that is that Sonatype's release plugin insists on cloning the entire repository for a compile using whatever is behing XPATH /project/scm/developerConnection -- my connection is poor, and this plugin seems to forget what a git SHA1 is).

OK, all of this rant to ask: how do you integrate the library to your project exactly? I think I can learn a lot here.

So, yes, I am reopening this issue, although this is not related to the original bug anymore -- this time, it is I asking for your input ;)

@anupam2502
Copy link

In my code I am using ProcessingReport report = schema.validate(jsonNode); There is no compilation issue and when I run the test from the same project it is not giving any exception. But when I execute the same by adding dependency of my current project to another project and then running the test from that project, I am getting the following error : java.lang.NoSuchMethodError: com.github.fge.jsonschema.main.JsonSchema.validate(Lcom/fasterxml/jackson/databind/JsonNode;)Lcom/github/fge/jsonschema/core/report/ProcessingReport;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants