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

JENA-1258 NodeFmtLib calls JenaSystem initializer #189

Closed
wants to merge 2 commits into from

Conversation

stain
Copy link
Member

@stain stain commented Nov 7, 2016

Fixes JENA-1258 in NodeFmtLib

Note that the added TestNodeFmtInit test case would not normally find the bug ran from Maven as earlier tests would have caused initialization, but it can be tested from say Eclipse.

@afs
Copy link
Member

afs commented Nov 7, 2016

This usage is calling into the middle of internal code.

Fixing output code seems wrong. It is better to initialize as objects get created, not on output. This shows the root cause is somewhere else.

@stain
Copy link
Member Author

stain commented Nov 7, 2016

Except for impl packages and suffixes it is never easy for Jena users to know what is "internal code".

If it's in Javadoc, and it is useful "outside" without much hazzle, then I don't see why it should be called "internal" just because it is also used internally.

Agree that it would be good to ensure Jena initialization from NodeFactory.

@afs
Copy link
Member

afs commented Nov 7, 2016

The root cause:

Commit 204f253 switched Context to using a ConcurrentHashMap, commit d67a426 added a check for null otherwise nothing has changed since 3.1.0.

@afs
Copy link
Member

afs commented Nov 8, 2016

The better fix is to initialize in NodeFactory (and Node for legacy reasons) only. I have tested this in a build.

There is little point in a specific test because tests run in the overall suite; it's almost confusing to have a test that doesn't. With initialization in NodeFactory this will get tested anyway!

Also, it would help to unroll ARQConstants (for RDF, RDFS, OWL, XSD) so it does not need JenaSystem.init and then it is, with care, init-safe.

See PR#192 for that all made real.

The original example works when run separately to ensure minimal system initialization.

@stain
Copy link
Member Author

stain commented Nov 8, 2016

Yes, your pull request looks like the best approach.

On 8 Nov 2016 12:33 pm, "Andy Seaborne" notifications@github.com wrote:

The better fix is to initialize in NodeFactory (and Node for legacy
reasons) only. I have tested this in a build.

There is little point in a specific test because tests run in the overall
suite; it's almost confusing to have a test that doesn't. With
initialization in NodeFactory this will get tested anyway!

Also, it would help to unroll ARQConstants (for RDF, RDFS, OWL, XSD) so it
does not need JenaSystem.init and then it is, with care, init-safe.

See PR#192 for that all made real.

The original example works when run separately to ensure minimal system
initialization.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#189 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAPd5fFN_RTsft_0zoh3lFvprRyNmYrsks5q8Gu3gaJpZM4Kqwha
.

@asfgit asfgit closed this in 874c3be Nov 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants