The test fails, because the @DataSourceDefinition is used on a POJO - i.e. it's not a JndiConsumer, hence the ConvertDataSourceDefinitions deployer cannot know that such definition exists. I'll have to check the spec if it mentions which classes can be annotated with @DataSourceDefinition, but either way it would be easy to fix. I'll take a look tomorrow :)
Kind regards, Svetlin 2017-06-16 22:37 GMT+03:00 Romain Manni-Bucau <rmannibu...@gmail.com>: > Trunk seems to have an issue with this in the test > XADataSourceDefinitionTest > > Want to have a look? > > > Romain Manni-Bucau > @rmannibucau <https://twitter.com/rmannibucau> | Blog > <https://blog-rmannibucau.rhcloud.com> | Old Blog > <http://rmannibucau.wordpress.com> | Github <https://github.com/ > rmannibucau> | > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory > <https://javaeefactory-rmannibucau.rhcloud.com> > > 2017-06-16 20:46 GMT+02:00 Romain Manni-Bucau <rmannibu...@gmail.com>: > > > applied, thanks! > > > > > > Romain Manni-Bucau > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > > <https://blog-rmannibucau.rhcloud.com> | Old Blog > > <http://rmannibucau.wordpress.com> | Github > > <https://github.com/rmannibucau> | LinkedIn > > <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory > > <https://javaeefactory-rmannibucau.rhcloud.com> > > > > 2017-06-16 20:33 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail. > com> > > : > > > >> I've fixed the license headers. Here is the PR: > >> https://github.com/apache/tomee/pull/73 > >> > >> I didn't check JMS, only WepProfile specs. > >> > >> 2017-06-16 21:01 GMT+03:00 Romain Manni-Bucau <rmannibu...@gmail.com>: > >> > >> > This looks good, while the merge deployer still makes available the > >> > resource to comp it works :) > >> > > >> > Think you need to fix details on your PR like headers etc but looks ok > >> to > >> > merge once the build pass. > >> > > >> > Side note: by curiosity, did you check jms resource as well? it should > >> be > >> > close ot datasources. > >> > > >> > > >> > Romain Manni-Bucau > >> > @rmannibucau <https://twitter.com/rmannibucau> | Blog > >> > <https://blog-rmannibucau.rhcloud.com> | Old Blog > >> > <http://rmannibucau.wordpress.com> | Github <https://github.com/ > >> > rmannibucau> | > >> > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory > >> > <https://javaeefactory-rmannibucau.rhcloud.com> > >> > > >> > 2017-06-16 19:56 GMT+02:00 Svetlin Zarev > <svetlin.angelov.zarev@gmail.c > >> om > >> > >: > >> > > >> > > I'm back with tests:) : > >> > > https://github.com/apache/tomee/compare/master... > >> > > SvetlinZarev:ds_def?expand=1 > >> > > > >> > > Unless I misunderstood you, it seems that DS injection in servlet > >> works > >> > > after the fix and does not before it (well, it injects it, but with > >> wrong > >> > > config). > >> > > The reason it works is that the ConvertDataSourceDefinitions > deployer > >> > only > >> > > processes the DataSource definitions by converting them to Resource > >> > objects > >> > > which are not associated with specific component, and since the > >> > > CompManagedBean does not have its own, unique definitions, if we > >> exclude > >> > it > >> > > from processing in that specific deployer we won't lose anything, > >> because > >> > > data sources with the same IDs ( and with correct config !) will be > >> > pulled > >> > > from the JndiConsumers that actually provide them. > >> > > > >> > > So to summarize, the flow before the proposed fix is: > >> > > 1. Collect all JndiConsumers > >> > > 2. MyBean -> provide DataSource with correct config > >> > > 3. CompManagedBean -> overwrite MyBean's datasource, with one with > bad > >> > > config > >> > > 4. We end up with one resource with bad config > >> > > > >> > > And after the fix it becomes: > >> > > 1. Collect all JndiConsumers > >> > > 2. MyBean -> provide DataSource with correct config > >> > > 3. We end up with one resource with good config > >> > > > >> > > I hope I don't miss anything :) May you give more details about your > >> > idea ? > >> > > > >> > > Kind regards, > >> > > Svetlin > >> > > > >> > > > >> > > 2017-06-16 16:32 GMT+03:00 Romain Manni-Bucau < > rmannibu...@gmail.com > >> >: > >> > > > >> > > > Hmm, if you inject such a datasource in a servlet does it work? > >> Don't > >> > > think > >> > > > so - tests are the thing to start with when editing this code, > >> saying > >> > > that > >> > > > by experience if you get me ;). > >> > > > > >> > > > So concretely testing the type like that is good but it shouldn't > >> break > >> > > web > >> > > > component injections. > >> > > > > >> > > > > >> > > > Romain Manni-Bucau > >> > > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > >> > > > <https://blog-rmannibucau.rhcloud.com> | Old Blog > >> > > > <http://rmannibucau.wordpress.com> | Github <https://github.com/ > >> > > > rmannibucau> | > >> > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE > Factory > >> > > > <https://javaeefactory-rmannibucau.rhcloud.com> > >> > > > > >> > > > 2017-06-16 15:27 GMT+02:00 Svetlin Zarev > >> <svetlin.angelov.zarev@gmail. > >> > > com > >> > > > >: > >> > > > > >> > > > > Hi, > >> > > > > > >> > > > > I was thinking about something like > >> > > > > https://github.com/SvetlinZarev/tomee/commit/ > >> > > > > 067fd220e909e89a8c17d90122b0e2158468ece4 > >> > > > > > >> > > > > What do you think ? Is there a more appropriate place to check > for > >> > it ? > >> > > > > If it's OK, I can make PR with the fix + tests. > >> > > > > > >> > > > > Thanks, > >> > > > > Svetlin > >> > > > > > >> > > > > > >> > > > > 2017-06-16 16:15 GMT+03:00 Romain Manni-Bucau < > >> rmannibu...@gmail.com > >> > >: > >> > > > > > >> > > > > > Hi Svetlin > >> > > > > > > >> > > > > > this is a way to aggregate the webapp java:comp/env namespace > >> > without > >> > > > > > handling it too specifically in the code base - at least it > >> comes > >> > > from > >> > > > > that > >> > > > > > idea. > >> > > > > > > >> > > > > > We can add it in EjbJar and just skip it at deploy time (we do > >> > > > something > >> > > > > > similar already, don't recally exactly where but it is typed > >> enough > >> > > to > >> > > > > know > >> > > > > > it is the comp bean). > >> > > > > > > >> > > > > > Does it give you enough input to work on it or do you want > some > >> > > > > particular > >> > > > > > code reference? > >> > > > > > > >> > > > > > > >> > > > > > Romain Manni-Bucau > >> > > > > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > >> > > > > > <https://blog-rmannibucau.rhcloud.com> | Old Blog > >> > > > > > <http://rmannibucau.wordpress.com> | Github < > >> https://github.com/ > >> > > > > > rmannibucau> | > >> > > > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE > >> > Factory > >> > > > > > <https://javaeefactory-rmannibucau.rhcloud.com> > >> > > > > > > >> > > > > > 2017-06-16 15:10 GMT+02:00 Svetlin Zarev > >> > > <svetlin.angelov.zarev@gmail. > >> > > > > com > >> > > > > > >: > >> > > > > > > >> > > > > > > Hi Everyone, > >> > > > > > > > >> > > > > > > What's the purpose of the org.apache.openejb.config. > >> > > CompManagedBean > >> > > > ? > >> > > > > > I'm > >> > > > > > > asking in the context of TOMEE-2053. > >> > > > > > > > >> > > > > > > I have a @DataSourceDefinition with some attributes which > >> should > >> > be > >> > > > > > > overrriden by ejb-jar.xml. Everithing works great, with the > >> sole > >> > > > > > exception > >> > > > > > > of CompManagedBean. It seems that it "aggregates" the > >> annotations > >> > > > from > >> > > > > > the > >> > > > > > > other beans, but as it's artificially added to the ejb-jar > by > >> > > > openejb, > >> > > > > it > >> > > > > > > does not have an entry in the ejb-jar.xml. Hence when the > >> > > > > > > AnnotationDeployer processes the DataSourceDefinition > >> annotation, > >> > > if > >> > > > > > never > >> > > > > > > finds an exisitin datasource definition in the ejb-jar.xml > for > >> > it. > >> > > > This > >> > > > > > in > >> > > > > > > turn makes the annotation deployer to add a datasource with > >> wrong > >> > > > > > > configuration to the AppModule's ejb-jar. So far so good, > but > >> > > later, > >> > > > > the > >> > > > > > > ConvertDataSourceDefinitions deployer collects all > datasources > >> > from > >> > > > all > >> > > > > > > JndiConsumers, so it collects the invalid definition as well > >> and > >> > > adds > >> > > > > it > >> > > > > > to > >> > > > > > > the AppModule's resources. And this breaks the application > >> > startup. > >> > > > > > > > >> > > > > > > Kind regards, > >> > > > > > > Svetlin > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > >
