Re: CompManagedBean & @DatasourceDefinition - TOMEE-2053

2017-06-19 Thread Romain Manni-Bucau
big fingers :(

thanks for the catch, should be fixed. Build in progress at
https://ci.apache.org/builders/tomee-trunk-ubuntu/builds/777


Romain Manni-Bucau
@rmannibucau  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | JavaEE Factory


2017-06-19 9:20 GMT+02:00 Svetlin Zarev :

> Hi,
>
> The DataSourcePojo.java is missing from master, and the arquilian tests
> fail to compile. May you add it:
> https://github.com/apache/tomee/pull/74/commits/
> 26ee7018df455c3a5b46c6a0b87adb38feeab34f#diff-
> c89f954c712eb9ab263f8acf8d75586f
> ?
>
> Kind regadrs,
> Svetlin
>
> 2017-06-17 11:13 GMT+03:00 Romain Manni-Bucau :
>
> > You rock Svetlin, applied, thanks a lot!
> >
> >
> > Romain Manni-Bucau
> > @rmannibucau  |  Blog
> >  | Old Blog
> >  | Github  > rmannibucau> |
> > LinkedIn  | JavaEE Factory
> > 
> >
> > 2017-06-17 7:56 GMT+02:00 Svetlin Zarev  >:
> >
> > > Hi,
> > >
> > > Here it is: https://github.com/apache/tomee/pull/74
> > >
> > > Kind regards,
> > > Svetlin
> > >
> > >
> > > 2017-06-16 23:44 GMT+03:00 Svetlin Zarev  > com
> > > >:
> > >
> > > > 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  >:
> > > >
> > > >> Trunk seems to have an issue with this in the test
> > > >> XADataSourceDefinitionTest
> > > >>
> > > >> Want to have a look?
> > > >>
> > > >>
> > > >> Romain Manni-Bucau
> > > >> @rmannibucau  |  Blog
> > > >>  | Old Blog
> > > >>  | Github <
> > > >> https://github.com/rmannibucau> |
> > > >> LinkedIn  | JavaEE Factory
> > > >> 
> > > >>
> > > >> 2017-06-16 20:46 GMT+02:00 Romain Manni-Bucau <
> rmannibu...@gmail.com
> > >:
> > > >>
> > > >> > applied, thanks!
> > > >> >
> > > >> >
> > > >> > Romain Manni-Bucau
> > > >> > @rmannibucau  |  Blog
> > > >> >  | Old Blog
> > > >> >  | Github
> > > >> >  | LinkedIn
> > > >> >  | JavaEE Factory
> > > >> > 
> > > >> >
> > > >> > 2017-06-16 20:33 GMT+02:00 Svetlin Zarev
> > >  > > >> om>
> > > >> > :
> > > >> >
> > > >> >> 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  |  Blog
> > > >> >> >  | Old Blog
> > > >> >> >  | Github <
> https://github.com/
> > > >> >> > rmannibucau> |
> > > >> >> > LinkedIn  | JavaEE
> > > Factory
> > > >> >> > 
> > > >> >> >
> > > >> >> > 2017-06-16 19:56 GMT+02:00 Svetlin Zarev
> > > >>  > > >> >> 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 

Re: CompManagedBean & @DatasourceDefinition - TOMEE-2053

2017-06-19 Thread Svetlin Zarev
Hi,

The DataSourcePojo.java is missing from master, and the arquilian tests
fail to compile. May you add it:
https://github.com/apache/tomee/pull/74/commits/26ee7018df455c3a5b46c6a0b87adb38feeab34f#diff-c89f954c712eb9ab263f8acf8d75586f
?

Kind regadrs,
Svetlin

2017-06-17 11:13 GMT+03:00 Romain Manni-Bucau :

> You rock Svetlin, applied, thanks a lot!
>
>
> Romain Manni-Bucau
> @rmannibucau  |  Blog
>  | Old Blog
>  | Github  rmannibucau> |
> LinkedIn  | JavaEE Factory
> 
>
> 2017-06-17 7:56 GMT+02:00 Svetlin Zarev :
>
> > Hi,
> >
> > Here it is: https://github.com/apache/tomee/pull/74
> >
> > Kind regards,
> > Svetlin
> >
> >
> > 2017-06-16 23:44 GMT+03:00 Svetlin Zarev  com
> > >:
> >
> > > 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 :
> > >
> > >> Trunk seems to have an issue with this in the test
> > >> XADataSourceDefinitionTest
> > >>
> > >> Want to have a look?
> > >>
> > >>
> > >> Romain Manni-Bucau
> > >> @rmannibucau  |  Blog
> > >>  | Old Blog
> > >>  | Github <
> > >> https://github.com/rmannibucau> |
> > >> LinkedIn  | JavaEE Factory
> > >> 
> > >>
> > >> 2017-06-16 20:46 GMT+02:00 Romain Manni-Bucau  >:
> > >>
> > >> > applied, thanks!
> > >> >
> > >> >
> > >> > Romain Manni-Bucau
> > >> > @rmannibucau  |  Blog
> > >> >  | Old Blog
> > >> >  | Github
> > >> >  | LinkedIn
> > >> >  | JavaEE Factory
> > >> > 
> > >> >
> > >> > 2017-06-16 20:33 GMT+02:00 Svetlin Zarev
> >  > >> om>
> > >> > :
> > >> >
> > >> >> 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  |  Blog
> > >> >> >  | Old Blog
> > >> >> >  | Github  > >> >> > rmannibucau> |
> > >> >> > LinkedIn  | JavaEE
> > Factory
> > >> >> > 
> > >> >> >
> > >> >> > 2017-06-16 19:56 GMT+02:00 Svetlin Zarev
> > >>  > >> >> 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 

Re: CompManagedBean & @DatasourceDefinition - TOMEE-2053

2017-06-17 Thread Romain Manni-Bucau
You rock Svetlin, applied, thanks a lot!


Romain Manni-Bucau
@rmannibucau  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | JavaEE Factory


2017-06-17 7:56 GMT+02:00 Svetlin Zarev :

> Hi,
>
> Here it is: https://github.com/apache/tomee/pull/74
>
> Kind regards,
> Svetlin
>
>
> 2017-06-16 23:44 GMT+03:00 Svetlin Zarev  >:
>
> > 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 :
> >
> >> Trunk seems to have an issue with this in the test
> >> XADataSourceDefinitionTest
> >>
> >> Want to have a look?
> >>
> >>
> >> Romain Manni-Bucau
> >> @rmannibucau  |  Blog
> >>  | Old Blog
> >>  | Github <
> >> https://github.com/rmannibucau> |
> >> LinkedIn  | JavaEE Factory
> >> 
> >>
> >> 2017-06-16 20:46 GMT+02:00 Romain Manni-Bucau :
> >>
> >> > applied, thanks!
> >> >
> >> >
> >> > Romain Manni-Bucau
> >> > @rmannibucau  |  Blog
> >> >  | Old Blog
> >> >  | Github
> >> >  | LinkedIn
> >> >  | JavaEE Factory
> >> > 
> >> >
> >> > 2017-06-16 20:33 GMT+02:00 Svetlin Zarev
>  >> om>
> >> > :
> >> >
> >> >> 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  >:
> >> >>
> >> >> > 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  |  Blog
> >> >> >  | Old Blog
> >> >> >  | Github  >> >> > rmannibucau> |
> >> >> > LinkedIn  | JavaEE
> Factory
> >> >> > 
> >> >> >
> >> >> > 2017-06-16 19:56 GMT+02:00 Svetlin Zarev
> >>  >> >> 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
> >> >> > >
> >> 

Re: CompManagedBean & @DatasourceDefinition - TOMEE-2053

2017-06-16 Thread Svetlin Zarev
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 :

> Trunk seems to have an issue with this in the test
> XADataSourceDefinitionTest
>
> Want to have a look?
>
>
> Romain Manni-Bucau
> @rmannibucau  |  Blog
>  | Old Blog
>  | Github  rmannibucau> |
> LinkedIn  | JavaEE Factory
> 
>
> 2017-06-16 20:46 GMT+02:00 Romain Manni-Bucau :
>
> > applied, thanks!
> >
> >
> > Romain Manni-Bucau
> > @rmannibucau  |  Blog
> >  | Old Blog
> >  | Github
> >  | LinkedIn
> >  | JavaEE Factory
> > 
> >
> > 2017-06-16 20:33 GMT+02:00 Svetlin Zarev  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 :
> >>
> >> > 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  |  Blog
> >> >  | Old Blog
> >> >  | Github  >> > rmannibucau> |
> >> > LinkedIn  | JavaEE Factory
> >> > 
> >> >
> >> > 2017-06-16 19:56 GMT+02:00 Svetlin Zarev
>  >> 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  |  Blog
> >> > > >  | Old Blog
> >> > > >  | Github  >> > > > rmannibucau> |
> >> > > > LinkedIn  | 

Re: CompManagedBean & @DatasourceDefinition - TOMEE-2053

2017-06-16 Thread Romain Manni-Bucau
Trunk seems to have an issue with this in the test
XADataSourceDefinitionTest

Want to have a look?


Romain Manni-Bucau
@rmannibucau  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | JavaEE Factory


2017-06-16 20:46 GMT+02:00 Romain Manni-Bucau :

> applied, thanks!
>
>
> Romain Manni-Bucau
> @rmannibucau  |  Blog
>  | Old Blog
>  | Github
>  | LinkedIn
>  | JavaEE Factory
> 
>
> 2017-06-16 20:33 GMT+02:00 Svetlin Zarev 
> :
>
>> 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 :
>>
>> > 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  |  Blog
>> >  | Old Blog
>> >  | Github > > rmannibucau> |
>> > LinkedIn  | JavaEE Factory
>> > 
>> >
>> > 2017-06-16 19:56 GMT+02:00 Svetlin Zarev > 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 > >:
>> > >
>> > > > 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  |  Blog
>> > > >  | Old Blog
>> > > >  | Github > > > > rmannibucau> |
>> > > > LinkedIn  | JavaEE Factory
>> > > > 
>> > > >
>> > > > 2017-06-16 15:27 GMT+02:00 Svetlin Zarev
>> > > > 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
>> > >:
>> > > > >
>> > > 

Re: CompManagedBean & @DatasourceDefinition - TOMEE-2053

2017-06-16 Thread Romain Manni-Bucau
applied, thanks!


Romain Manni-Bucau
@rmannibucau  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | JavaEE Factory


2017-06-16 20:33 GMT+02:00 Svetlin Zarev :

> 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 :
>
> > 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  |  Blog
> >  | Old Blog
> >  | Github  > rmannibucau> |
> > LinkedIn  | JavaEE Factory
> > 
> >
> > 2017-06-16 19:56 GMT+02:00 Svetlin Zarev  com
> > >:
> >
> > > 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 :
> > >
> > > > 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  |  Blog
> > > >  | Old Blog
> > > >  | Github  > > > rmannibucau> |
> > > > LinkedIn  | JavaEE Factory
> > > > 
> > > >
> > > > 2017-06-16 15:27 GMT+02:00 Svetlin Zarev
>  > > 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?
> > > > > >
> > > > > >
> > > > > 

Re: CompManagedBean & @DatasourceDefinition - TOMEE-2053

2017-06-16 Thread Svetlin Zarev
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 :

> 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  |  Blog
>  | Old Blog
>  | Github  rmannibucau> |
> LinkedIn  | JavaEE Factory
> 
>
> 2017-06-16 19:56 GMT+02:00 Svetlin Zarev  >:
>
> > 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 :
> >
> > > 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  |  Blog
> > >  | Old Blog
> > >  | Github  > > rmannibucau> |
> > > LinkedIn  | JavaEE Factory
> > > 
> > >
> > > 2017-06-16 15:27 GMT+02:00 Svetlin Zarev  > 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  >:
> > > >
> > > > > 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  |  Blog
> > > > >  | Old Blog
> > > > >  | Github  > > > > rmannibucau> |
> > > > > LinkedIn  | JavaEE
> Factory
> > > > > 
> > > > >
> > > > > 2017-06-16 15:10 GMT+02:00 Svetlin Zarev
> >  > > > com
> > > > > >:
> > > > >
> > > > > > Hi Everyone,
> > > > > >
> > > > > > What's the purpose of the org.apache.openejb.config.
> > CompManagedBean
> > > ?
> > > > > I'm
> > > > > > asking in the context 

Re: CompManagedBean & @DatasourceDefinition - TOMEE-2053

2017-06-16 Thread Romain Manni-Bucau
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  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | JavaEE Factory


2017-06-16 19:56 GMT+02:00 Svetlin Zarev :

> 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 :
>
> > 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  |  Blog
> >  | Old Blog
> >  | Github  > rmannibucau> |
> > LinkedIn  | JavaEE Factory
> > 
> >
> > 2017-06-16 15:27 GMT+02:00 Svetlin Zarev  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 :
> > >
> > > > 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  |  Blog
> > > >  | Old Blog
> > > >  | Github  > > > rmannibucau> |
> > > > LinkedIn  | JavaEE Factory
> > > > 
> > > >
> > > > 2017-06-16 15:10 GMT+02:00 Svetlin Zarev
>  > > 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 

Re: CompManagedBean & @DatasourceDefinition - TOMEE-2053

2017-06-16 Thread Svetlin Zarev
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 :

> 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  |  Blog
>  | Old Blog
>  | Github  rmannibucau> |
> LinkedIn  | JavaEE Factory
> 
>
> 2017-06-16 15:27 GMT+02:00 Svetlin Zarev  >:
>
> > 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 :
> >
> > > 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  |  Blog
> > >  | Old Blog
> > >  | Github  > > rmannibucau> |
> > > LinkedIn  | JavaEE Factory
> > > 
> > >
> > > 2017-06-16 15:10 GMT+02:00 Svetlin Zarev  > 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
> > > >
> > >
> >
>


Re: CompManagedBean & @DatasourceDefinition - TOMEE-2053

2017-06-16 Thread Romain Manni-Bucau
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  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | JavaEE Factory


2017-06-16 15:27 GMT+02:00 Svetlin Zarev :

> 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 :
>
> > 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  |  Blog
> >  | Old Blog
> >  | Github  > rmannibucau> |
> > LinkedIn  | JavaEE Factory
> > 
> >
> > 2017-06-16 15:10 GMT+02:00 Svetlin Zarev  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
> > >
> >
>


Re: CompManagedBean & @DatasourceDefinition - TOMEE-2053

2017-06-16 Thread Svetlin Zarev
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 :

> 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  |  Blog
>  | Old Blog
>  | Github  rmannibucau> |
> LinkedIn  | JavaEE Factory
> 
>
> 2017-06-16 15:10 GMT+02:00 Svetlin Zarev  >:
>
> > 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
> >
>


Re: CompManagedBean & @DatasourceDefinition - TOMEE-2053

2017-06-16 Thread Romain Manni-Bucau
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  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | JavaEE Factory


2017-06-16 15:10 GMT+02:00 Svetlin Zarev :

> 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
>