You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomee.apache.org by Svetlin Zarev <sv...@gmail.com> on 2017/02/07 09:31:30 UTC
OpenEJB custom configuration factory & bval.in-container=true
Hello,
In *one* of the configuration factory constructors a system property is set:
public ConfigurationFactory() {
this(!shouldAutoDeploy());
System.setProperty("bval.in-container", "true");
}
What do you think about moving this to system.properties ? The reason is
that:
* it's really hidden and not obvious why it's there
* it's being set every time in case of vanilla tomee
* it's not set when using custom configuration factory when you use one of
the other constructors (and this breaks CDI)
* IMO it's a good idea to have all global properties at one place, instead
of being scattered in the source
Kind regards,
Svetlin
Re: OpenEJB custom configuration factory & bval.in-container=true
Posted by Romain Manni-Bucau <rm...@gmail.com>.
2017-02-07 11:56 GMT+01:00 Svetlin Zarev <sv...@gmail.com>:
> > Side note (find it funny): you complain this property is skipped if you
> use
> > another constructor but same applies to the auto deploy one ;)
>
> The auto deploy one (the no-arg constructor) sets this property :) While
> the others - do not. Which results in a very unpleasant surprise when using
> custom configuration factory.
well you hardcode it so still surprising for users having configured it ;)
anyway not the central piece of that thread - yet ;)
> So may idea was to make the behavior
> consistent among the different constructors. Also it's much easier to
> override or check the value of the property if it's in system.properties,
> rather than searching throughout the whole codebase. When I tried CDI
> enabled app on my tomee with custom configuration factory, the deployment
> started to fail because there were two BeanValidator classes and it was
> pure luck that I accidentally discovered that hidden property.
>
>
Hmm should fail cause Validator injection is ambiguous, the other issues
are not linked to that property but BValCdiFilter implementation which is
set by default whatever this system property is
> Svetlin
>
> 2017-02-07 11:41 GMT+02:00 Romain Manni-Bucau <rm...@gmail.com>:
>
> > Hi
> >
> > 2017-02-07 10:31 GMT+01:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.
> com
> > >:
> >
> > > Hello,
> > >
> > > In *one* of the configuration factory constructors a system property is
> > > set:
> > >
> > > public ConfigurationFactory() {
> > > this(!shouldAutoDeploy());
> > > System.setProperty("bval.in-container", "true");
> > > }
> > >
> > > What do you think about moving this to system.properties ? The reason
> is
> > > that:
> > >
> >
> > well, not an option cause 80% of "tomee" usages are not in tomee and
> don't
> > have system.properties - but doesn't mean we can't move it, just that the
> > file will not work
> >
> >
> > > * it's really hidden and not obvious why it's there
> > >
> >
> > Was the most common piece of code executed early enough when added and
> also
> > a place you can override and change if you desire to keep default
> behavior
> > or have a custom one. We can surely make it part of SystemInstance but
> > override will be harder.
> >
> >
> > > * it's being set every time in case of vanilla tomee
> > >
> >
> > Was one of the goal, not sure I see what the issue is
> >
> >
> > > * it's not set when using custom configuration factory when you use one
> > of
> > > the other constructors (and this breaks CDI)
> > >
> >
> > Not set: was part of the goal to let the custom factory change that
> easily
> > but agree we can find a better place
> > Breaks CDI: depends the CDI config actually but by default yes cause
> > BValExtension will try to add a validator and validator factory but
> openejb
> > does it as well
> >
> >
> > > * IMO it's a good idea to have all global properties at one place,
> > instead
> > > of being scattered in the source
> > >
> >
> > Yes and no, there are multiple kind of properties:
> >
> > - global one ("enforced") like this one: not against aggregating them but
> > gain shouldn't be that big
> > - speific ones: this needs to be done per code area and not aggregated
> all
> > together (you will not put tomee properties in openejb code for instance)
> > - "in case ones": we have some of that kind where it is there just cause
> it
> > can break and it enables the user to make its app working even if default
> > is not desired. These ones are not all very official and I don't find it
> > that bad to hide them a bit cause we shouldn't encourage them
> >
> > So story short: we can aggregate several ones but we can't - I think -
> put
> > them all in a single SystemProperties class.
> >
> > Side note (find it funny): you complain this property is skipped if you
> use
> > another constructor but same applies to the auto deploy one ;)
> >
> > Finally note official properties are listed on
> > http://tomee.apache.org/admin/configuration/server.html
> >
> > If we miss a bit we can add them but this bval one is an internal one. At
> > some point we can even drop it from bval and just detect we run in tomee
> > and set it this way instead of using a system property - maybe better?
> >
> >
> > >
> > > Kind regards,
> > > Svetlin
> > >
> >
>
Re: OpenEJB custom configuration factory & bval.in-container=true
Posted by Svetlin Zarev <sv...@gmail.com>.
> Side note (find it funny): you complain this property is skipped if you
use
> another constructor but same applies to the auto deploy one ;)
The auto deploy one (the no-arg constructor) sets this property :) While
the others - do not. Which results in a very unpleasant surprise when using
custom configuration factory. So may idea was to make the behavior
consistent among the different constructors. Also it's much easier to
override or check the value of the property if it's in system.properties,
rather than searching throughout the whole codebase. When I tried CDI
enabled app on my tomee with custom configuration factory, the deployment
started to fail because there were two BeanValidator classes and it was
pure luck that I accidentally discovered that hidden property.
Svetlin
2017-02-07 11:41 GMT+02:00 Romain Manni-Bucau <rm...@gmail.com>:
> Hi
>
> 2017-02-07 10:31 GMT+01:00 Svetlin Zarev <svetlin.angelov.zarev@gmail.com
> >:
>
> > Hello,
> >
> > In *one* of the configuration factory constructors a system property is
> > set:
> >
> > public ConfigurationFactory() {
> > this(!shouldAutoDeploy());
> > System.setProperty("bval.in-container", "true");
> > }
> >
> > What do you think about moving this to system.properties ? The reason is
> > that:
> >
>
> well, not an option cause 80% of "tomee" usages are not in tomee and don't
> have system.properties - but doesn't mean we can't move it, just that the
> file will not work
>
>
> > * it's really hidden and not obvious why it's there
> >
>
> Was the most common piece of code executed early enough when added and also
> a place you can override and change if you desire to keep default behavior
> or have a custom one. We can surely make it part of SystemInstance but
> override will be harder.
>
>
> > * it's being set every time in case of vanilla tomee
> >
>
> Was one of the goal, not sure I see what the issue is
>
>
> > * it's not set when using custom configuration factory when you use one
> of
> > the other constructors (and this breaks CDI)
> >
>
> Not set: was part of the goal to let the custom factory change that easily
> but agree we can find a better place
> Breaks CDI: depends the CDI config actually but by default yes cause
> BValExtension will try to add a validator and validator factory but openejb
> does it as well
>
>
> > * IMO it's a good idea to have all global properties at one place,
> instead
> > of being scattered in the source
> >
>
> Yes and no, there are multiple kind of properties:
>
> - global one ("enforced") like this one: not against aggregating them but
> gain shouldn't be that big
> - speific ones: this needs to be done per code area and not aggregated all
> together (you will not put tomee properties in openejb code for instance)
> - "in case ones": we have some of that kind where it is there just cause it
> can break and it enables the user to make its app working even if default
> is not desired. These ones are not all very official and I don't find it
> that bad to hide them a bit cause we shouldn't encourage them
>
> So story short: we can aggregate several ones but we can't - I think - put
> them all in a single SystemProperties class.
>
> Side note (find it funny): you complain this property is skipped if you use
> another constructor but same applies to the auto deploy one ;)
>
> Finally note official properties are listed on
> http://tomee.apache.org/admin/configuration/server.html
>
> If we miss a bit we can add them but this bval one is an internal one. At
> some point we can even drop it from bval and just detect we run in tomee
> and set it this way instead of using a system property - maybe better?
>
>
> >
> > Kind regards,
> > Svetlin
> >
>
Re: OpenEJB custom configuration factory & bval.in-container=true
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hi
2017-02-07 10:31 GMT+01:00 Svetlin Zarev <sv...@gmail.com>:
> Hello,
>
> In *one* of the configuration factory constructors a system property is
> set:
>
> public ConfigurationFactory() {
> this(!shouldAutoDeploy());
> System.setProperty("bval.in-container", "true");
> }
>
> What do you think about moving this to system.properties ? The reason is
> that:
>
well, not an option cause 80% of "tomee" usages are not in tomee and don't
have system.properties - but doesn't mean we can't move it, just that the
file will not work
> * it's really hidden and not obvious why it's there
>
Was the most common piece of code executed early enough when added and also
a place you can override and change if you desire to keep default behavior
or have a custom one. We can surely make it part of SystemInstance but
override will be harder.
> * it's being set every time in case of vanilla tomee
>
Was one of the goal, not sure I see what the issue is
> * it's not set when using custom configuration factory when you use one of
> the other constructors (and this breaks CDI)
>
Not set: was part of the goal to let the custom factory change that easily
but agree we can find a better place
Breaks CDI: depends the CDI config actually but by default yes cause
BValExtension will try to add a validator and validator factory but openejb
does it as well
> * IMO it's a good idea to have all global properties at one place, instead
> of being scattered in the source
>
Yes and no, there are multiple kind of properties:
- global one ("enforced") like this one: not against aggregating them but
gain shouldn't be that big
- speific ones: this needs to be done per code area and not aggregated all
together (you will not put tomee properties in openejb code for instance)
- "in case ones": we have some of that kind where it is there just cause it
can break and it enables the user to make its app working even if default
is not desired. These ones are not all very official and I don't find it
that bad to hide them a bit cause we shouldn't encourage them
So story short: we can aggregate several ones but we can't - I think - put
them all in a single SystemProperties class.
Side note (find it funny): you complain this property is skipped if you use
another constructor but same applies to the auto deploy one ;)
Finally note official properties are listed on
http://tomee.apache.org/admin/configuration/server.html
If we miss a bit we can add them but this bval one is an internal one. At
some point we can even drop it from bval and just detect we run in tomee
and set it this way instead of using a system property - maybe better?
>
> Kind regards,
> Svetlin
>