I am proposing a change to the org.hibernate.jpa.packaging.spi.Scanner SPI. First I suggest we make the scanning happen in one call, and that one call return a token of the result. Currently we have 4 "scan" methods on the Scanner contract; I instead propose there be a ScanResult that holds those 4 results: interface ScanResult { public Set<String> getPackageNames(); public Set<String> getClassNames(); public Set<NamedInputStream> getResourceStreams(); }

java.lang.Package for returning the matching classes and packages. This facilitates the "late loading" of those on the classloader (jandex/classmate).

interface Scanner { public ScanResult scan(PersistenceUnitDescriptor descriptor, ScanOptions options); } or maybe: interface Scanner { public ScanResult scan(URL rootUrl, ScanOptions options); } though I definitely prefer the first option. So the first thing to notice here is that there is one call to the scanner for the entire persistence unit, not the multiple calls we do today. So the onus of scanning sub-jars and "other jars" moves to the Scanner. This is helpful in OSGi environments. It also allows easier caching on the Scanner impl side for deployment walking. Lots of other little wins here IMO too. ScanOptions essentially just allows us a way to pass in the things we want to limit on; search filters if you will.

I'd like to make this change in 4.3 (JPA 2.1 support), so please speak up sooner rather than later if you have disagree with this proposal. Comments, suggestions, improvements welcome.

_______________________________________________ hibernate-dev mailing list hibernate-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

On Sat 16 Mar 2013 03:29:22 AM CDT, Gunnar Morling wrote: > > interface ScanResult { > public Set<String> getPackageNames(); > public Set<String> getClassNames(); > public Set<NamedInputStream> getResourceStreams(); > } > > > Is there missing one method? Or is it 4 methods in the original design > and one is not required with the new design? The latter. The current Scanner contract defined: Set<NamedInputStream> getFilesInJar(URL jartoScan, Set<String> filePatterns); Set<NamedInputStream> getFilesInClasspath(Set<String> filePatterns); I consolidated this into one return group. > > The first thing to note is the move away from using > java.lang.Class and > java.lang.Package for returning the matching classes and packages. > This > facilitates the "late loading" of those on the classloader > (jandex/classmate). > > > Just a thought: would it make sense to return something like a > ClassDescriptor or PackageDescriptor which would know how to > materialize a given name into a Class or Package (by having methods > such as asClass() etc.)? This might be helpful as it encapsulates > which class loader to use and also avoids to accidentally use package > names as class names etc. I like that suggestion. Not sure it makes sense have it encapsulate the link back to the classloader for loading. But either way I definitely like the idea of the specific return type. > > ScanOptions essentially just allows us a way to pass in the things we > want to limit on; search filters if you will. > > > Is ScanOptions available somewhere already? Based on the name I assume > this can contain several options. Would it alternatively make sense to > have something like ScanOption... options? The problem is that the different options apply to each "bucket" of returns. So a simple list would certainly not work. Now it is questionable whether the options are needed for the package and class name scanning. The existing code essentially allows configurable set of annotations to limit the search: Set<Package> getPackagesInJar(URL jartoScan, Set<Class<? extends Annotation>> annotationsToLookFor); Set<Class<?>> getClassesInJar(URL jartoScan, Set<Class<? extends Annotation>> annotationsToLookFor); but currently we always pass empty to getPackagesInJar and a static set of values to getClassesInJar. So, to me it is questionable whether that is really needed. The scanner could just do these since the code calling scanner never varies these. Now, for getResourceStreams[1] if we drop the notion of any options for getPackageNames and getClassNames I can see the param being just a varargs/array of "scan option" objects. But to me, this highlights the niceness of "parameter objects". If we leave it as ScanOption and initially leave off options for getPackageNames and getClassNames but later decide to add it, the implementations of Scanner do not need to change. [1] changing my mind to calling that getNamedInputStreams instead.

It's probably best if the SPI would take the ClassLoader as parameter, instead of having to guess it, or depend on proper TCCL (think OSGi ;-)). e.g. for this reason Infinispan introduced AdvancedCache::withClassLoader. -Ales On Mar 16, 2013, at 8:48 PM, Steve Ebersole <steve(a)hibernate.org&gt; wrote: > I should clarify why I did not necessarily like the ClassDescriptor and > PackageDescriptor (or whatever we name them) to "encapsulate which > class loader to use". At least based on the JBoss AS usage (and I can > envision other environments being similar) we do not necessarily know > the ultimate ClassLoader for these things when we are doing the > scanning. That ClassLoader instance gets built later. > > On Sat 16 Mar 2013 09:53:19 AM CDT, Steve Ebersole wrote: >> On Sat 16 Mar 2013 03:29:22 AM CDT, Gunnar Morling wrote: >>> interface ScanResult { >>> public Set<String> getPackageNames(); >>> public Set<String> getClassNames(); >>> public Set<NamedInputStream> getResourceStreams(); >>> } >>> >>> >>> Is there missing one method? Or is it 4 methods in the original design >>> and one is not required with the new design? >> The latter. The current Scanner contract defined: >> >> Set<NamedInputStream> getFilesInJar(URL jartoScan, Set<String> >> filePatterns); >> Set<NamedInputStream> getFilesInClasspath(Set<String> filePatterns); >> >> I consolidated this into one return group. >> >> >>> The first thing to note is the move away from using >>> java.lang.Class and >>> java.lang.Package for returning the matching classes and packages. >>> This >>> facilitates the "late loading" of those on the classloader >>> (jandex/classmate). >>> >>> >>> Just a thought: would it make sense to return something like a >>> ClassDescriptor or PackageDescriptor which would know how to >>> materialize a given name into a Class or Package (by having methods >>> such as asClass() etc.)? This might be helpful as it encapsulates >>> which class loader to use and also avoids to accidentally use package >>> names as class names etc. >> I like that suggestion. Not sure it makes sense have it encapsulate >> the link back to the classloader for loading. But either way I >> definitely like the idea of the specific return type. >> >> >>> ScanOptions essentially just allows us a way to pass in the things we >>> want to limit on; search filters if you will. >>> >>> >>> Is ScanOptions available somewhere already? Based on the name I assume >>> this can contain several options. Would it alternatively make sense to >>> have something like ScanOption... options? >> The problem is that the different options apply to each "bucket" of >> returns. So a simple list would certainly not work. Now it is >> questionable whether the options are needed for the package and class >> name scanning. The existing code essentially allows configurable set >> of annotations to limit the search: >> >> Set<Package> getPackagesInJar(URL jartoScan, Set<Class<? extends >> Annotation>> annotationsToLookFor); >> Set<Class<?>> getClassesInJar(URL jartoScan, Set<Class<? extends >> Annotation>> annotationsToLookFor); >> >> but currently we always pass empty to getPackagesInJar and a static >> set of values to getClassesInJar. So, to me it is questionable >> whether that is really needed. The scanner could just do these since >> the code calling scanner never varies these. >> >> Now, for getResourceStreams[1] if we drop the notion of any options >> for getPackageNames and getClassNames I can see the param being just a >> varargs/array of "scan option" objects. But to me, this highlights >> the niceness of "parameter objects". If we leave it as ScanOption and >> initially leave off options for getPackageNames and getClassNames but >> later decide to add it, the implementations of Scanner do not need to >> change. >> >> >> [1] changing my mind to calling that getNamedInputStreams instead. > _______________________________________________ > hibernate-dev mailing list > hibernate-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev _______________________________________________ hibernate-dev mailing list hibernate-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

I should point out this is based on what we saw when Brett initially worked with Karaf, especially in the Enterprise OSGi use cases. The incoming PersistenceUnitInfo contained no urls other than the root url, which happened to be an osgi bundle url (the protocol was "bundle"). To me, interpreting all these goofy url schemes is best left to the environment that defines those schemes. Not much unlike JBoss AS and its VFS-based urls.

Set<Package> getPackagesInJar(URL jartoScan, Set<Class<? extends Annotation>> annotationsToLookFor); Set<Class<?>> getClassesInJar(URL jartoScan, Set<Class<? extends Annotation>> annotationsToLookFor); but currently we always pass empty to getPackagesInJar and a static set of values to getClassesInJar. So, to me it is questionable whether that is really needed. The scanner could just do these since the code calling scanner never varies these. Now, for getResourceStreams[1] if we drop the notion of any options for getPackageNames and getClassNames I can see the param being just a varargs/array of "scan option" objects. But to me, this highlights the niceness of "parameter objects". I didn't mean that ScanOption should not be a parameter object, I was just wondering why it is one ScanOption*s* object instead of a list/var-args of option objects.

I am proposing a change to the org.hibernate.jpa.packaging.spi.Scanner SPI. First I suggest we make the scanning happen in one call, and that one call return a token of the result. Currently we have 4 "scan" methods on the Scanner contract; I instead propose there be a ScanResult that holds those 4 results: interface ScanResult { public Set<String> getPackageNames(); public Set<String> getClassNames(); public Set<NamedInputStream> getResourceStreams(); } The first thing to note is the move away from using java.lang.Class and java.lang.Package for returning the matching classes and packages. This facilitates the "late loading" of those on the classloader (jandex/classmate). To get a ScanResult, we'd call the newly consolidated method on Scanner: interface Scanner { public ScanResult scan(PersistenceUnitDescriptor descriptor, ScanOptions options); } or maybe: interface Scanner { public ScanResult scan(URL rootUrl, ScanOptions options); } though I definitely prefer the first option. So the first thing to notice here is that there is one call to the scanner for the entire persistence unit, not the multiple calls we do today. So the onus of scanning sub-jars and "other jars" moves to the Scanner. This is helpful in OSGi environments. It also allows easier caching on the Scanner impl side for deployment walking. Lots of other little wins here IMO too. ScanOptions essentially just allows us a way to pass in the things we want to limit on; search filters if you will. I'd like to make this change in 4.3 (JPA 2.1 support), so please speak up sooner rather than later if you have disagree with this proposal. Comments, suggestions, improvements welcome. _______________________________________________ hibernate-dev mailing list hibernate-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

On Mon 18 Mar 2013 05:14:01 AM CDT, Emmanuel Bernard wrote: > >JBoss AS does use this contract so if you break it, we will need some >kind of compatibility matrix between Hibernate and JBoss AS and EAP. >Not unsurmountable but always a small annoyance. >Maybe other environments also make use of this interface but I am not >aware of them. As far as JBoss AS, Scott has been involved in this design from the beginning. >I'm surprised getUnqualifiedJarName is no longer needed. I thought we >used it as the default PU name but the current code does not use >getUnqualifiedJarName >anymore. I have never seen that #getUnqualifiedJarName used aside from tests. >We initially designed the Scanner interface to minimize the work the >Scanner implementor has to do and keep as much of the JPA knowledge to >HEM's code. Your design seems to require the Scanner to understand more of >JPA including the notion of root jar and additional jar files. There is actually very very very little "JPA knowledge" being asked of the Scanner in my proposal. Keep in mind that in both the cases that have surfaced so far where we actually need "custom Scanner" both are cases where the Scanner provider is also the thing that is handing us the root/additional jars. For EE JPA thats actually part of the PersistenceUnitInfo contract; no magic there. So for JBoss AS (or another AS) to hand us both the PersistenceUnitInfo (with jar urls) and the Scanner (knowning how to scan said url protocols) is not unreasonable. In the case of Enterprise OSGi (at least based on our initial target environment), we have a PersistenceUnitInfo that only points us to the root url (#getJarFileUrls returns nothing), but this is the kind of "environment specifics" the current implementation forces Hibernate to understand. And then, in both cases it forces Hibernate to import and use non-standard APIs just to do the scanning (JBoss's VirtualFile contract and quite a few OSGi contracts). The important point I think you are missing is that it is far more difficult asking Hibernate to understand all the url protocol schemes in play then for the environments using those protocols to tell use how to scan them.

>Things around: > >- getMappingFileNames to return the stream for these files, Not at all following here. Do you mean getMappingFileNames on the PersistenceUnitInfo? Well that does *not* return streams, it returns Strings. And the spec specifically says that the Strings are supposed to be the resource names of the mapping files (aka, they should be loadable by that name through ClassLoader). So what exactly is the point here?

>- isExcludeUnlistedClasses to not scan classes in the root JAR, Exactly. This "option" only has bearing on the root jar. For all other jars Hibernate tries to be friendly and load everything. But, that is hardly "deep JPA knowledge". The option in terms of the root jars maps directly to an explicit JPA discussion. Nothing deep about the knowledge there. And for the non-root jars, there is nothing JPA specific in this option; its purely a Hibernate *choice*.

>- getJarFileUrls Again, I think you are missing the point that generally speaking the PersistenceUnitInfo provider and the Scanner provider are one-in-the-same. >- look for META-INF/orm.xml in the root JAr (only) and exclude it if it >is already listed explicitly in the getMappingFileNames to not process >it twice. Not sure how this is classified as "deep JPA knowledge".

>- getManagedClassNames depending on how much you delegate to the scanner Again, not sure how this is classified as "deep JPA knowledge". I assume you mean because of PUI#excludeUnlistedClasses, but see that discussion above. >That makes me concerned about code duplication and bugs unless someone >deep in JPA immplement all of these Scanner implementations. So, I am really not seeing this "need for deep knowledge of JPA" on the Scanner implementor in what I propose.

On Mon 18 Mar 2013 09:10:40 AM CDT, Emmanuel Bernard wrote: > > That was the point of the Scanning API in the first place: not to depend > on specific APIs like the VirtualFile. That and the fact that JBoss AS > had Jandex and we did not were the reason for Ales and me to have this > interface. Ok, but the decision to ask the Scanner these questions for each jar is inadvertently requiring Hibernate to be sensitive to these protocols. In fact the current standard Scanner impl has explicit handling for VFS protocol schemes. And for OSGi, it would require that the single root URL (because again thats all we get) actually encompasses the root jar *and* any other specified jars. Aka, `if ( "bundle".equals( urlProtocol ) ) { doSomethingDrasticallyDifferent(); }...`. So take the EE case, which I know is true of JBoss. JBoss is the one handing us the PersistenceUnitInfo (which is the thing that backs the PersistenceUnitDescriptor we pass back to it in this Scanner proposal). I don't think it is unreasonable that JBoss understands the difference between PersistenceUnitInfo#getPersistenceUnitRootUrl and PersistenceUnitInfo#getJarFileUrls intrinsically (after all, it is the one that gave us the PUI). By intrinsically I mean that it already understands that there is a difference/distinction between the root and any non-roots. So from there the question really is whether expecting the Scanner to apply different scanning rules to root versus not-root URLs constitutes "deep JPA knowledge". And maybe that is different for each type of thing for which we scan. For classes e.g. nothing stops us from saying the the Scanner just returns us everything it finds and that we then filter or weed through it. For example, if it returns us descriptors for all the classes it finds, we can apply the "only use that class if it (a) came from a specified jar or (b) came from the root and #excludeUnlistedClasses allows us to use them" logic ourselves. Thats a trivial matter of us defining the ClassDescriptor contract to tell us whether the class was found in the root or not: interface ClassDescriptor { public String getClassName(); public NamedInputStream getNamedInputStream(); public boolean foundInRoot(); } But again, I dont think that the Scanner applying that simple logic is really "deep JPA knowledge". But if everyone else agrees it is, we can easily alleviate that via the above pattern. As for resources (orm.xml and hbm.xml files), I'd have to look a little closer maybe. To be honest that is some of the code that uses the "Map as an API" approach that makes me want to rip out my hair, gouge out my eyes and leap off the nearest bridge :) It is code like this code that has made me want to re-write what was EJB3Configuration from scratch ever since I first started looking at it. That and the fact that any time I ask anyone that had a hand in writing that code anything about that code they simply throw up their hands and say they don't know ;) Maybe it is more complicated than I am making it out to be. But I don't think the way this is then presented to the rest of the code needs to be complex. At the end of the day, there should be a scan and the results of that scan should be a nice easy-to-understand contract. > The previous implementation of Scanner was returning the resource > streams so you had to take PUI.getMappingFileNames() into account. I got > confused. Still not following. The proposed Scanner contract is returning NamedInputStreams, which is exactly what the current Scanner returns for these. > No that's what the JPA spec says (or intended), additional jars are > to be scanned to > find the entity classes. Otherwise the additional jar feature becomes > very limited or even useless. You would only need it otherwise to > retrieve the XML DD which was considered a second class citizen on > JPA 1.0. Ok, but is that "deep JPA knowledge" beyond what a typical JPA container developer would generally know? >>> - look for META-INF/orm.xml in the root JAr (only) and exclude it >>> if it >>> is already listed explicitly in the getMappingFileNames to not process >>> it twice. >> >> Not sure how this is classified as "deep JPA knowledge". > > > Understanding and implementing all these rules took me a while :) Ok, don't include duplicates.. check. ;) Not saying the rules are simplistic. Just questioning whether they are only known to those with "deep JPA knowledge". Not so sure that is true. > I gave you my opinion, if you think that implementing Scanner will > remain simple enough then most of my remarks are moot. Considering I will implement the 3 main Scanner impls... ;) But seriously, I don't think delegating to Scanner just once per persistence unit puts undue difficulty on the Scanner impl.

On Mon 18 Mar 2013 05:14:01 AM CDT, Emmanuel Bernard wrote: > > JBoss AS does use this contract so if you break it, we will need some > kind of compatibility matrix between Hibernate and JBoss AS and EAP. > Not unsurmountable but always a small annoyance. > Maybe other environments also make use of this interface but I am not > aware of them. As far as JBoss AS, Scott has been involved in this design from the beginning.

> I'm surprised getUnqualifiedJarName is no longer needed. I thought we > used it as the default PU name but the current code does not use > getUnqualifiedJarName > anymore. I have never seen that #getUnqualifiedJarName used aside from tests. > We initially designed the Scanner interface to minimize the work the > Scanner implementor has to do and keep as much of the JPA knowledge to > HEM's code. Your design seems to require the Scanner to understand more of > JPA including the notion of root jar and additional jar files. There is actually very very very little "JPA knowledge" being asked of the Scanner in my proposal. Keep in mind that in both the cases that have surfaced so far where we actually need "custom Scanner" both are cases where the Scanner provider is also the thing that is handing us the root/additional jars. For EE JPA thats actually part of the PersistenceUnitInfo contract; no magic there. So for JBoss AS (or another AS) to hand us both the PersistenceUnitInfo (with jar urls) and the Scanner (knowning how to scan said url protocols) is not unreasonable. In the case of Enterprise OSGi (at least based on our initial target environment), we have a PersistenceUnitInfo that only points us to the root url (#getJarFileUrls returns nothing), but this is the kind of "environment specifics" the current implementation forces Hibernate to understand. And then, in both cases it forces Hibernate to import and use non-standard APIs just to do the scanning (JBoss's VirtualFile contract and quite a few OSGi contracts). The important point I think you are missing is that it is far more difficult asking Hibernate to understand all the url protocol schemes in play then for the environments using those protocols to tell use how to scan them. > Things around: > > - getMappingFileNames to return the stream for these files, Not at all following here. Do you mean getMappingFileNames on the PersistenceUnitInfo? Well that does *not* return streams, it returns Strings. And the spec specifically says that the Strings are supposed to be the resource names of the mapping files (aka, they should be loadable by that name through ClassLoader). So what exactly is the point here? > - isExcludeUnlistedClasses to not scan classes in the root JAR, Exactly. This "option" only has bearing on the root jar. For all other jars Hibernate tries to be friendly and load everything. But, that is hardly "deep JPA knowledge". The option in terms of the root jars maps directly to an explicit JPA discussion. Nothing deep about the knowledge there. And for the non-root jars, there is nothing JPA specific in this option; its purely a Hibernate *choice*. > - getJarFileUrls Again, I think you are missing the point that generally speaking the PersistenceUnitInfo provider and the Scanner provider are one-in-the-same. > - look for META-INF/orm.xml in the root JAr (only) and exclude it if it > is already listed explicitly in the getMappingFileNames to not process > it twice. Not sure how this is classified as "deep JPA knowledge". > - getManagedClassNames depending on how much you delegate to the scanner Again, not sure how this is classified as "deep JPA knowledge". I assume you mean because of PUI#excludeUnlistedClasses, but see that discussion above. > That makes me concerned about code duplication and bugs unless someone > deep in JPA immplement all of these Scanner implementations. So, I am really not seeing this "need for deep knowledge of JPA" on the Scanner implementor in what I propose. _______________________________________________ hibernate-dev mailing list hibernate-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

On 03/18/2013 09:15 AM, Steve Ebersole wrote: > On Mon 18 Mar 2013 05:14:01 AM CDT, Emmanuel Bernard wrote: >> >> JBoss AS does use this contract so if you break it, we will need some >> kind of compatibility matrix between Hibernate and JBoss AS and EAP. >> Not unsurmountable but always a small annoyance. >> Maybe other environments also make use of this interface but I am not >> aware of them. > > As far as JBoss AS, Scott has been involved in this design from the > beginning. Given all of the discussion so far, and feedback from Ales/Emmanuel who originally created the Scanner, I want to back track and reassess before we get too far ahead on changing the scanner (from the point ofo view of the AS side, just to confirm that the new design would work). My initial observation was that the AS side is returning Class instances that we are only getting the name from. Just returning the name directly might be better. However, if Hibernate does need to access to the classes, I'm also fine with continuing to return the classes. Sorry that I have been absent from this thread. My queue of other stuff is building up. Scott > >> I'm surprised getUnqualifiedJarName is no longer needed. I thought we >> used it as the default PU name but the current code does not use >> getUnqualifiedJarName >> anymore. > > I have never seen that #getUnqualifiedJarName used aside from tests. > >> We initially designed the Scanner interface to minimize the work the >> Scanner implementor has to do and keep as much of the JPA knowledge to >> HEM's code. Your design seems to require the Scanner to understand >> more of >> JPA including the notion of root jar and additional jar files. > > There is actually very very very little "JPA knowledge" being asked of > the Scanner in my proposal. Keep in mind that in both the cases that > have surfaced so far where we actually need "custom Scanner" both are > cases where the Scanner provider is also the thing that is handing us > the root/additional jars. For EE JPA thats actually part of the > PersistenceUnitInfo contract; no magic there. So for JBoss AS (or > another AS) to hand us both the PersistenceUnitInfo (with jar urls) and > the Scanner (knowning how to scan said url protocols) is not > unreasonable. In the case of Enterprise OSGi (at least based on our > initial target environment), we have a PersistenceUnitInfo that only > points us to the root url (#getJarFileUrls returns nothing), but this is > the kind of "environment specifics" the current implementation forces > Hibernate to understand. And then, in both cases it forces Hibernate to > import and use non-standard APIs just to do the scanning (JBoss's > VirtualFile contract and quite a few OSGi contracts). The important > point I think you are missing is that it is far more difficult asking > Hibernate to understand all the url protocol schemes in play then for > the environments using those protocols to tell use how to scan them. > > >> Things around: >> >> - getMappingFileNames to return the stream for these files, > Not at all following here. Do you mean getMappingFileNames on the > PersistenceUnitInfo? Well that does *not* return streams, it returns > Strings. And the spec specifically says that the Strings are supposed > to be the resource names of the mapping files (aka, they should be > loadable by that name through ClassLoader). So what exactly is the > point here? > >> - isExcludeUnlistedClasses to not scan classes in the root JAR, > Exactly. This "option" only has bearing on the root jar. For all other > jars Hibernate tries to be friendly and load everything. But, that is > hardly "deep JPA knowledge". The option in terms of the root jars maps > directly to an explicit JPA discussion. Nothing deep about the > knowledge there. And for the non-root jars, there is nothing JPA > specific in this option; its purely a Hibernate *choice*. > >> - getJarFileUrls > Again, I think you are missing the point that generally speaking the > PersistenceUnitInfo provider and the Scanner provider are > one-in-the-same. > >> - look for META-INF/orm.xml in the root JAr (only) and exclude it if it >> is already listed explicitly in the getMappingFileNames to not process >> it twice. > Not sure how this is classified as "deep JPA knowledge". > >> - getManagedClassNames depending on how much you delegate to the >> scanner > Again, not sure how this is classified as "deep JPA knowledge". I > assume you mean because of PUI#excludeUnlistedClasses, but see that > discussion above. > >> That makes me concerned about code duplication and bugs unless someone >> deep in JPA immplement all of these Scanner implementations. > > So, I am really not seeing this "need for deep knowledge of JPA" on the > Scanner implementor in what I propose. > _______________________________________________ > hibernate-dev mailing list > hibernate-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev >

The problem is what Hibernate does once it returns from the Scanner calls. It applies filters. And the filters is applies are different based on whether the url being scanned is the PU root url or a non-root. You simply don't have that info; so I don't think it will work. On Tue 19 Mar 2013 10:49:56 AM CDT, Brett Meyer wrote: > It's probably too early in the process to know for sure, but I'm > already headed down the path of using the existing Scanner contract > for OSGi scanning in 4.2. I provide a custom Scanner that ignores > the "jar URL" completely -- all scans tap into the OSGi > BundleWiring. The scanning methods shouldn't be called repeatedly > since PersistenceUnitInfo#getJarFileUrls is empty (at least in the > Karaf container). Correct me if that won't work... > > Brett Meyer > Red Hat Software Engineer, Hibernate > +1 260.349.5732 > > ----- Original Message ----- > From: "Steve Ebersole" <steve(a)hibernate.org&gt; > To: "Scott Marlow" <smarlow(a)redhat.com&gt; > Cc: "Hibernate hibernate-dev" <hibernate-dev(a)lists.jboss.org&gt; > Sent: Tuesday, March 19, 2013 11:19:09 AM > Subject: Re: [hibernate-dev] Scanner contract > > Yes, part of the redesign was to return class names (and streams) > rather than Class instances, but thats just part of the reasoning. > > The problem is that we really can't continue to use the Scanner > contract as-is; it is not great for OSGi environments. Could we hack > up the OSGi stuff to work with Scanner? I don't think so. When OSGi > PersistenceUnitInfo simply returns you a root URL, how can we possibly > apply different filters for root/non-root using the existing Scanner > contract? > > Anyway, I am well down the path of implementing this. I'll push to my > fork when done and everyone can chime in their on real concrete code. > One thing I can tell you that will be a huge bugbear here in terms of > refactoring is that this existing Scanner and JarVisitor code has > *zero* unit tests. > > > On Tue 19 Mar 2013 10:06:42 AM CDT, Scott Marlow wrote: >> On 03/18/2013 09:15 AM, Steve Ebersole wrote: >>> On Mon 18 Mar 2013 05:14:01 AM CDT, Emmanuel Bernard wrote: >>>> >>>> JBoss AS does use this contract so if you break it, we will need some >>>> kind of compatibility matrix between Hibernate and JBoss AS and EAP. >>>> Not unsurmountable but always a small annoyance. >>>> Maybe other environments also make use of this interface but I am not >>>> aware of them. >>> >>> As far as JBoss AS, Scott has been involved in this design from the >>> beginning. >> >> Given all of the discussion so far, and feedback from Ales/Emmanuel >> who originally created the Scanner, I want to back track and reassess >> before we get too far ahead on changing the scanner (from the point >> ofo view of the AS side, just to confirm that the new design would >> work). My initial observation was that the AS side is returning Class >> instances that we are only getting the name from. Just returning the >> name directly might be better. However, if Hibernate does need to >> access to the classes, I'm also fine with continuing to return the >> classes. >> >> Sorry that I have been absent from this thread. My queue of other >> stuff is building up. >> >> Scott >> >>> >>>> I'm surprised getUnqualifiedJarName is no longer needed. I thought we >>>> used it as the default PU name but the current code does not use >>>> getUnqualifiedJarName >>>> anymore. >>> >>> I have never seen that #getUnqualifiedJarName used aside from tests. >>> >>>> We initially designed the Scanner interface to minimize the work the >>>> Scanner implementor has to do and keep as much of the JPA >>>> knowledge to >>>> HEM's code. Your design seems to require the Scanner to understand >>>> more of >>>> JPA including the notion of root jar and additional jar files. >>> >>> There is actually very very very little "JPA knowledge" being asked of >>> the Scanner in my proposal. Keep in mind that in both the cases that >>> have surfaced so far where we actually need "custom Scanner" both are >>> cases where the Scanner provider is also the thing that is handing us >>> the root/additional jars. For EE JPA thats actually part of the >>> PersistenceUnitInfo contract; no magic there. So for JBoss AS (or >>> another AS) to hand us both the PersistenceUnitInfo (with jar urls) >>> and >>> the Scanner (knowning how to scan said url protocols) is not >>> unreasonable. In the case of Enterprise OSGi (at least based on our >>> initial target environment), we have a PersistenceUnitInfo that only >>> points us to the root url (#getJarFileUrls returns nothing), but >>> this is >>> the kind of "environment specifics" the current implementation forces >>> Hibernate to understand. And then, in both cases it forces >>> Hibernate to >>> import and use non-standard APIs just to do the scanning (JBoss's >>> VirtualFile contract and quite a few OSGi contracts). The important >>> point I think you are missing is that it is far more difficult asking >>> Hibernate to understand all the url protocol schemes in play then for >>> the environments using those protocols to tell use how to scan them. >>> >>> >>>> Things around: >>>> >>>> - getMappingFileNames to return the stream for these files, >>> Not at all following here. Do you mean getMappingFileNames on the >>> PersistenceUnitInfo? Well that does *not* return streams, it returns >>> Strings. And the spec specifically says that the Strings are supposed >>> to be the resource names of the mapping files (aka, they should be >>> loadable by that name through ClassLoader). So what exactly is the >>> point here? >>> >>>> - isExcludeUnlistedClasses to not scan classes in the root JAR, >>> Exactly. This "option" only has bearing on the root jar. For all >>> other >>> jars Hibernate tries to be friendly and load everything. But, that is >>> hardly "deep JPA knowledge". The option in terms of the root jars >>> maps >>> directly to an explicit JPA discussion. Nothing deep about the >>> knowledge there. And for the non-root jars, there is nothing JPA >>> specific in this option; its purely a Hibernate *choice*. >>> >>>> - getJarFileUrls >>> Again, I think you are missing the point that generally speaking the >>> PersistenceUnitInfo provider and the Scanner provider are >>> one-in-the-same. >>> >>>> - look for META-INF/orm.xml in the root JAr (only) and exclude it >>>> if it >>>> is already listed explicitly in the getMappingFileNames to not >>>> process >>>> it twice. >>> Not sure how this is classified as "deep JPA knowledge". >>> >>>> - getManagedClassNames depending on how much you delegate to the >>>> scanner >>> Again, not sure how this is classified as "deep JPA knowledge". I >>> assume you mean because of PUI#excludeUnlistedClasses, but see that >>> discussion above. >>> >>>> That makes me concerned about code duplication and bugs unless >>>> someone >>>> deep in JPA immplement all of these Scanner implementations. >>> >>> So, I am really not seeing this "need for deep knowledge of JPA" on >>> the >>> Scanner implementor in what I propose. >>> _______________________________________________ >>> hibernate-dev mailing list >>> hibernate-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/hibernate-dev >>> >> > _______________________________________________ > hibernate-dev mailing list > hibernate-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev

I should point out that for most cases, writing a new Scanner should be as simple as extending org.hibernate.jpa.boot.scan.spi.AbstractScannerImpl and passing in a custom org.hibernate.jpa.boot.archive.spi.ArchiveDescriptorFactory that handles specific archive types (by URL protocol, etc). For OSGi, we will need a tiny bit more work for environments like we have seen that only give us the root url. There, assuming we extend AbstractScannerImpl, we will need the custom ArchiveDescriptorFactory and somehow it will need to account for the single url -> multiple artifacts deal (probably the ArchiveDescriptors it returns will need to do some delegation to sub-ArchiveDescriptors). On Wed 20 Mar 2013 07:39:51 PM CDT, Steve Ebersole wrote: > Here is the initial work I did on this. > > https://github.com/sebersole/hibernate-core/tree/HHH-8088 > > Unfortunately it includes a lot of fixes to bad test code, especially > in org.hibernate.jpa.test.packaging.PackagingTestCase and children > (please no more relative file references in tests). > > At a high level, scanning involves mainly collaboration between stuff > in 2 packages: > 1) org.hibernate.jpa.boot.archive - essentially what used to be > JarVisitor and friends > 2) org.hibernate.jpa.boot.scan - uses the archive "walking" provided > by org.hibernate.jpa.boot.archive and applies filtering to the "entries" > > The ultimate goal of the scan is to build a > org.hibernate.jpa.boot.scan.spi.ScanResult which essentially collects > together the org.hibernate.jpa.boot.spi.ClassDescriptor, > org.hibernate.jpa.boot.spi.PackageDescriptor and > org.hibernate.jpa.boot.spi.MappingFileDescriptor references. > > > On Tue 19 Mar 2013 11:02:38 AM CDT, Steve Ebersole wrote: >> The problem is what Hibernate does once it returns from the Scanner >> calls. It applies filters. And the filters is applies are different >> based on whether the url being scanned is the PU root url or a >> non-root. You simply don't have that info; so I don't think it will >> work. >> >> >> On Tue 19 Mar 2013 10:49:56 AM CDT, Brett Meyer wrote: >>> It's probably too early in the process to know for sure, but I'm >>> already headed down the path of using the existing Scanner contract >>> for OSGi scanning in 4.2. I provide a custom Scanner that ignores >>> the "jar URL" completely -- all scans tap into the OSGi >>> BundleWiring. The scanning methods shouldn't be called repeatedly >>> since PersistenceUnitInfo#getJarFileUrls is empty (at least in the >>> Karaf container). Correct me if that won't work... >>> >>> Brett Meyer >>> Red Hat Software Engineer, Hibernate >>> +1 260.349.5732 >>> >>> ----- Original Message ----- >>> From: "Steve Ebersole" <steve(a)hibernate.org&gt; >>> To: "Scott Marlow" <smarlow(a)redhat.com&gt; >>> Cc: "Hibernate hibernate-dev" <hibernate-dev(a)lists.jboss.org&gt; >>> Sent: Tuesday, March 19, 2013 11:19:09 AM >>> Subject: Re: [hibernate-dev] Scanner contract >>> >>> Yes, part of the redesign was to return class names (and streams) >>> rather than Class instances, but thats just part of the reasoning. >>> >>> The problem is that we really can't continue to use the Scanner >>> contract as-is; it is not great for OSGi environments. Could we hack >>> up the OSGi stuff to work with Scanner? I don't think so. When OSGi >>> PersistenceUnitInfo simply returns you a root URL, how can we possibly >>> apply different filters for root/non-root using the existing Scanner >>> contract? >>> >>> Anyway, I am well down the path of implementing this. I'll push to my >>> fork when done and everyone can chime in their on real concrete code. >>> One thing I can tell you that will be a huge bugbear here in terms of >>> refactoring is that this existing Scanner and JarVisitor code has >>> *zero* unit tests. >>> >>> >>> On Tue 19 Mar 2013 10:06:42 AM CDT, Scott Marlow wrote: >>>> On 03/18/2013 09:15 AM, Steve Ebersole wrote: >>>>> On Mon 18 Mar 2013 05:14:01 AM CDT, Emmanuel Bernard wrote: >>>>>> >>>>>> JBoss AS does use this contract so if you break it, we will need >>>>>> some >>>>>> kind of compatibility matrix between Hibernate and JBoss AS and >>>>>> EAP. >>>>>> Not unsurmountable but always a small annoyance. >>>>>> Maybe other environments also make use of this interface but I am >>>>>> not >>>>>> aware of them. >>>>> >>>>> As far as JBoss AS, Scott has been involved in this design from the >>>>> beginning. >>>> >>>> Given all of the discussion so far, and feedback from Ales/Emmanuel >>>> who originally created the Scanner, I want to back track and reassess >>>> before we get too far ahead on changing the scanner (from the point >>>> ofo view of the AS side, just to confirm that the new design would >>>> work). My initial observation was that the AS side is returning >>>> Class >>>> instances that we are only getting the name from. Just returning the >>>> name directly might be better. However, if Hibernate does need to >>>> access to the classes, I'm also fine with continuing to return the >>>> classes. >>>> >>>> Sorry that I have been absent from this thread. My queue of other >>>> stuff is building up. >>>> >>>> Scott >>>> >>>>> >>>>>> I'm surprised getUnqualifiedJarName is no longer needed. I >>>>>> thought we >>>>>> used it as the default PU name but the current code does not use >>>>>> getUnqualifiedJarName >>>>>> anymore. >>>>> >>>>> I have never seen that #getUnqualifiedJarName used aside from tests. >>>>> >>>>>> We initially designed the Scanner interface to minimize the work >>>>>> the >>>>>> Scanner implementor has to do and keep as much of the JPA >>>>>> knowledge to >>>>>> HEM's code. Your design seems to require the Scanner to understand >>>>>> more of >>>>>> JPA including the notion of root jar and additional jar files. >>>>> >>>>> There is actually very very very little "JPA knowledge" being >>>>> asked of >>>>> the Scanner in my proposal. Keep in mind that in both the cases >>>>> that >>>>> have surfaced so far where we actually need "custom Scanner" both >>>>> are >>>>> cases where the Scanner provider is also the thing that is >>>>> handing us >>>>> the root/additional jars. For EE JPA thats actually part of the >>>>> PersistenceUnitInfo contract; no magic there. So for JBoss AS (or >>>>> another AS) to hand us both the PersistenceUnitInfo (with jar urls) >>>>> and >>>>> the Scanner (knowning how to scan said url protocols) is not >>>>> unreasonable. In the case of Enterprise OSGi (at least based on our >>>>> initial target environment), we have a PersistenceUnitInfo that only >>>>> points us to the root url (#getJarFileUrls returns nothing), but >>>>> this is >>>>> the kind of "environment specifics" the current implementation >>>>> forces >>>>> Hibernate to understand. And then, in both cases it forces >>>>> Hibernate to >>>>> import and use non-standard APIs just to do the scanning (JBoss's >>>>> VirtualFile contract and quite a few OSGi contracts). The important >>>>> point I think you are missing is that it is far more difficult >>>>> asking >>>>> Hibernate to understand all the url protocol schemes in play then >>>>> for >>>>> the environments using those protocols to tell use how to scan them. >>>>> >>>>> >>>>>> Things around: >>>>>> >>>>>> - getMappingFileNames to return the stream for these files, >>>>> Not at all following here. Do you mean getMappingFileNames on the >>>>> PersistenceUnitInfo? Well that does *not* return streams, it >>>>> returns >>>>> Strings. And the spec specifically says that the Strings are >>>>> supposed >>>>> to be the resource names of the mapping files (aka, they should be >>>>> loadable by that name through ClassLoader). So what exactly is the >>>>> point here? >>>>> >>>>>> - isExcludeUnlistedClasses to not scan classes in the root JAR, >>>>> Exactly. This "option" only has bearing on the root jar. For all >>>>> other >>>>> jars Hibernate tries to be friendly and load everything. But, >>>>> that is >>>>> hardly "deep JPA knowledge". The option in terms of the root jars >>>>> maps >>>>> directly to an explicit JPA discussion. Nothing deep about the >>>>> knowledge there. And for the non-root jars, there is nothing JPA >>>>> specific in this option; its purely a Hibernate *choice*. >>>>> >>>>>> - getJarFileUrls >>>>> Again, I think you are missing the point that generally speaking the >>>>> PersistenceUnitInfo provider and the Scanner provider are >>>>> one-in-the-same. >>>>> >>>>>> - look for META-INF/orm.xml in the root JAr (only) and exclude it >>>>>> if it >>>>>> is already listed explicitly in the getMappingFileNames to not >>>>>> process >>>>>> it twice. >>>>> Not sure how this is classified as "deep JPA knowledge". >>>>> >>>>>> - getManagedClassNames depending on how much you delegate to the >>>>>> scanner >>>>> Again, not sure how this is classified as "deep JPA knowledge". I >>>>> assume you mean because of PUI#excludeUnlistedClasses, but see that >>>>> discussion above. >>>>> >>>>>> That makes me concerned about code duplication and bugs unless >>>>>> someone >>>>>> deep in JPA immplement all of these Scanner implementations. >>>>> >>>>> So, I am really not seeing this "need for deep knowledge of JPA" on >>>>> the >>>>> Scanner implementor in what I propose. >>>>> _______________________________________________ >>>>> hibernate-dev mailing list >>>>> hibernate-dev(a)lists.jboss.org >>>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev >>>>> >>>> >>> _______________________________________________ >>> hibernate-dev mailing list >>> hibernate-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/hibernate-dev