search

MorphiaOrg / morphia

MongoDB object-document mapper in Java based on https://github.com/mongodb/mongo-java-driver
Apache License 2.0
1.65k stars 456 forks source link

Illegal access on Instant field #1305

Closed pioorg closed 5 years ago

pioorg commented 5 years ago

Hi So basically I have an entity, which in turn has a field of type java.time.Instant. Morphia has a converter for this type, xyz.morphia.converters.InstantConverter.

When running the app in Java9+ (11 to be precise) with--illegal-access=deny, the app crashes, because during discovery performed by Datastore.ensureIndexes(EntityWithInstantField.class), in xyz.morphia.mapping.MappedClass#discover Morphia tries to set internal field (seconds) of Instant to be accessible. In the new module philosophy, this is deprecated and AFAICT will be removed in the future, to have nice and clean cross-module dependencies.

The exact exception is: An exception was caught and reported. Message: java.lang.reflect.InaccessibleObjectException: Unable to make field private final long java.time.Instant.seconds accessible: module java.base does not "opens java.time" to unnamed module @2617f816

So, if there's a registered codec for a class from an external module of which we don't have control, shouldn't Morphia skip discovering/accessing it using reflection?

BTW. I don't think any advice like "just permit the access" will hold in the future. While it works now, it may stop working in the future. (Mind that Thread.stop() finally got removed, just as EE modules.)

Any hints how this could be implemented, so I could give it a try?

evanchooly commented 5 years ago

I honestly haven't tried on anything post java8, yet. I've been focused on getting a proper 1.5.0 release done and then with the 2.0.0 release I haven't yet figured out if 8 or 11 would be a better target. I'm leaning toward 11 but I just haven't had time to work through that yet.

pioorg commented 5 years ago

Well, IMHO the Morphia should work with Java8 for time being, I think the migration to post8 will take some time here and there. However, it'd be a real pity if in the future modern Morphia wouldn't work... It should be no big deal for 12, but let's see what comes next ;-)

jyemin commented 5 years ago

Hey Justin,

Regarding Java 8 vs 11 as a minimum requirement, I'd lean towards 8. External surveys and our own (MongoDB's) data show that Java 8 is still the most widely adopted version, and Java 8 will be the minimum requirement for the upcoming 4.0 Java driver.

Regards, Jeff Yemin

On Wed, Dec 19, 2018 at 7:57 AM pioorg notifications@github.com wrote:

Well, IMHO the Morphia should work with Java8 for time being, I think the migration to post8 will take some time here and there. However, it'd be a real pity if in the future modern Morphia wouldn't work... It should be no big deal for 12, but let's see what comes next ;-)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/MorphiaOrg/morphia/issues/1305#issuecomment-448587625, or mute the thread https://github.com/notifications/unsubscribe-auth/ABD1-tWi_dW2xqNf_vTXIhi7Yk62-BMzks5u6jeugaJpZM4ZaDrC .

pioorg commented 5 years ago

I totally agree. However, it should also be kept in mind that people might use the driver and Morphia in Java 9+, especially 11+. There's also the concept of these multiversion JARs (to avoid java-x artifacts).

jyemin commented 5 years ago

Yeah, I didn't mean to imply that Morphia/Java driver only support Java 8, just that Java 8 should be the minimum at this time.

On Wed, Dec 19, 2018 at 9:00 AM pioorg notifications@github.com wrote:

I totally agree. However, it should also be kept in mind that people might use the driver and Morphia in Java 9+, especially 11+. There's also the concept of these multiversion JARs (to avoid java-x artifacts).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MorphiaOrg/morphia/issues/1305#issuecomment-448605880, or mute the thread https://github.com/notifications/unsubscribe-auth/ABD1-l9rLbtj-fK2vZERgXIR0eRceh-7ks5u6kZ2gaJpZM4ZaDrC .

evanchooly commented 5 years ago

Jeff, that's where I'm leaning too.
Java 11 doesn't really get us anything but hipster cred over what 8 gives. it's good to know the driver is moving to 8, actually Do you have a timeline for that? might be worth targeting morphia 2.0 for the 4.0 driver

evanchooly commented 5 years ago

I think we'll end up building in 11 but targeting 8. the move to 9 changed/broke the tagpet stuff, e.g. And of course the reflective restrictions need to mirigated.

jyemin commented 5 years ago

Nothing is set in stone, but we're targeting the second half of next year. You can see in Jira https://jira.mongodb.org/projects/JAVA/versions/19339 what we currently have planned for it.

The main thing for dependent libraries like Morphia is to remove usage of anything that's deprecated (though the legacy API will still be supported for the most part).

The 3.9 Upgrading Guide http://mongodb.github.io/mongo-java-driver/3.9/upgrading/ contains a good summary of our current plans.

Jeff

On Wed, Dec 19, 2018 at 11:53 AM Justin Lee notifications@github.com wrote:

I think we'll end up building in 11 but targeting 8. the move to 9 changed/broke the tagpet stuff, e.g. And of course the reflective restrictions need to mirigated.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MorphiaOrg/morphia/issues/1305#issuecomment-448666375, or mute the thread https://github.com/notifications/unsubscribe-auth/ABD1-ucVdC1Hdv6X0qBhc9Xy8WrQj8zMks5u6m8agaJpZM4ZaDrC .

evanchooly commented 5 years ago

good to know. i have a 2.x branch already reasonably modernized. I'm working on removing warts from morphia's API atm to make it easier to maintain going forward. I'm hoping i'll be done before 2019H2 :)

evanchooly commented 5 years ago

this isn't an issue under 2.0 using the provided codecs.

evanchooly commented 5 years ago

OK. so I know this is late but i've been playing with it. As it stands now, the cglib dependency is apparently not ready for this lockdown either. This library is required for lazy proxying of references. In 2.0, I've switched to bytebuddy for the lazy proxying and the codecs for handling these types do the right thing. Given the probable surgery required on the 1.x branch, i'm not entirely sure it's worth the effort just now. I'm going to close this issue in the meantime but should it become absolutely critical and 2.0 isn't an option at that point, we can revisit this one.