Re: open visibility of JavaClassCacheEntry

[Romain Manni-Bucau]

Fair enough, done in https://bz.apache.org/bugzilla/show_bug.cgi?id=62986

Romain Manni-Bucau
@rmannibucau  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | Book



Le jeu. 6 déc. 2018 à 14:32, Mark Thomas  a écrit :

> On 06/12/2018 13:19, Romain Manni-Bucau wrote:
> > Hello Rémy,
> >
> >
> > Le jeu. 6 déc. 2018 à 14:13, Rémy Maucherat  a écrit :
> >
> >> On Thu, Dec 6, 2018 at 11:51 AM Romain Manni-Bucau <
> rmannibu...@gmail.com>
> >> wrote:
> >>
> >>> Hi guys,
> >>>
> >>> can you make ContextConfig.JavaClassCacheEntry public please? Idea is
> to
> >> be
> >>> able to override ContextConfig and potentially customize
> >> processAnnotations
> >>> methods. Currently it is a pain and it is preventing to upgrade
> >> meecrowave
> >>> and likely tomee to java11+jlink support
> >>>
> >>
> >> Ok, so I had a look at the meecrowave main class after doing my embedded
> >> refactoring and I was a bit horrified by the amount of hacks :) Not that
> >> there were too many options to do the things that were done (some I
> didn't
> >> think were possible), and luckily my changes would have been a very good
> >> help there.
> >>
> >> So here, it looks like a bit scary as well, since JavaClassCacheEntry is
> >> package private (not ok usually), but only instantiated from a private
> >> method (what would you do about that ?). So I'd rather not do that, or
> not
> >> just that, since it would be good to reduce the amount of hacks.
> >>
> >> For starters, I don't like "private" in that kind of class (same with
> >> "final"), I prefer "protected" usually. Shouldn't JavaClassCacheEntry
> be a
> >> non static protected class ?
>
> No objections to making it protected if that helps but I don't see why
> you'd want to make it non-static.
>
> >> About webConfig, I'm not sure. A good way to do it would be to add
> calls to
> >> a few intermediate empty methods in appropriate locations, rather than a
> >> real refactoring.
> >
> > not empty cause here the idea is to reuse an existing scanning and not
> let
> > tomcat scan classes but yes a doScanMagic() would work
> > the part which would be great to reuse is the mapping between bytecode
> (the
> > annotation entry) and the tomcat model (FilterDef etc)
>
> Why don't you suggest a patch? As a rough guideline, please aim for the
> minimally invasive patch the gives you what you need.
>
> Generally adding to the existing API is OK but changing it is not. The
> aim is to minimise the chance of breakage for anyone currently using or
> extending that class directly.
>
> Mark
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>
>

Re: open visibility of JavaClassCacheEntry

[Mark Thomas]

On 06/12/2018 13:19, Romain Manni-Bucau wrote:
> Hello Rémy,
> 
> 
> Le jeu. 6 déc. 2018 à 14:13, Rémy Maucherat  a écrit :
> 
>> On Thu, Dec 6, 2018 at 11:51 AM Romain Manni-Bucau 
>> wrote:
>>
>>> Hi guys,
>>>
>>> can you make ContextConfig.JavaClassCacheEntry public please? Idea is to
>> be
>>> able to override ContextConfig and potentially customize
>> processAnnotations
>>> methods. Currently it is a pain and it is preventing to upgrade
>> meecrowave
>>> and likely tomee to java11+jlink support
>>>
>>
>> Ok, so I had a look at the meecrowave main class after doing my embedded
>> refactoring and I was a bit horrified by the amount of hacks :) Not that
>> there were too many options to do the things that were done (some I didn't
>> think were possible), and luckily my changes would have been a very good
>> help there.
>>
>> So here, it looks like a bit scary as well, since JavaClassCacheEntry is
>> package private (not ok usually), but only instantiated from a private
>> method (what would you do about that ?). So I'd rather not do that, or not
>> just that, since it would be good to reduce the amount of hacks.
>>
>> For starters, I don't like "private" in that kind of class (same with
>> "final"), I prefer "protected" usually. Shouldn't JavaClassCacheEntry be a
>> non static protected class ?

No objections to making it protected if that helps but I don't see why
you'd want to make it non-static.

>> About webConfig, I'm not sure. A good way to do it would be to add calls to
>> a few intermediate empty methods in appropriate locations, rather than a
>> real refactoring.
> 
> not empty cause here the idea is to reuse an existing scanning and not let
> tomcat scan classes but yes a doScanMagic() would work
> the part which would be great to reuse is the mapping between bytecode (the
> annotation entry) and the tomcat model (FilterDef etc)

Why don't you suggest a patch? As a rough guideline, please aim for the
minimally invasive patch the gives you what you need.

Generally adding to the existing API is OK but changing it is not. The
aim is to minimise the chance of breakage for anyone currently using or
extending that class directly.

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: open visibility of JavaClassCacheEntry

[Romain Manni-Bucau]

Hello Rémy,


Le jeu. 6 déc. 2018 à 14:13, Rémy Maucherat  a écrit :

> On Thu, Dec 6, 2018 at 11:51 AM Romain Manni-Bucau 
> wrote:
>
> > Hi guys,
> >
> > can you make ContextConfig.JavaClassCacheEntry public please? Idea is to
> be
> > able to override ContextConfig and potentially customize
> processAnnotations
> > methods. Currently it is a pain and it is preventing to upgrade
> meecrowave
> > and likely tomee to java11+jlink support
> >
>
> Ok, so I had a look at the meecrowave main class after doing my embedded
> refactoring and I was a bit horrified by the amount of hacks :) Not that
> there were too many options to do the things that were done (some I didn't
> think were possible), and luckily my changes would have been a very good
> help there.
>
> So here, it looks like a bit scary as well, since JavaClassCacheEntry is
> package private (not ok usually), but only instantiated from a private
> method (what would you do about that ?). So I'd rather not do that, or not
> just that, since it would be good to reduce the amount of hacks.
>
> For starters, I don't like "private" in that kind of class (same with
> "final"), I prefer "protected" usually. Shouldn't JavaClassCacheEntry be a
> non static protected class ?
>
> About webConfig, I'm not sure. A good way to do it would be to add calls to
> a few intermediate empty methods in appropriate locations, rather than a
> real refactoring.
>

not empty cause here the idea is to reuse an existing scanning and not let
tomcat scan classes but yes a doScanMagic() would work
the part which would be great to reuse is the mapping between bytecode (the
annotation entry) and the tomcat model (FilterDef etc)


>
> Rémy
>

Re: open visibility of JavaClassCacheEntry

[Rémy Maucherat]

On Thu, Dec 6, 2018 at 11:51 AM Romain Manni-Bucau 
wrote:

> Hi guys,
>
> can you make ContextConfig.JavaClassCacheEntry public please? Idea is to be
> able to override ContextConfig and potentially customize processAnnotations
> methods. Currently it is a pain and it is preventing to upgrade meecrowave
> and likely tomee to java11+jlink support
>

Ok, so I had a look at the meecrowave main class after doing my embedded
refactoring and I was a bit horrified by the amount of hacks :) Not that
there were too many options to do the things that were done (some I didn't
think were possible), and luckily my changes would have been a very good
help there.

So here, it looks like a bit scary as well, since JavaClassCacheEntry is
package private (not ok usually), but only instantiated from a private
method (what would you do about that ?). So I'd rather not do that, or not
just that, since it would be good to reduce the amount of hacks.

For starters, I don't like "private" in that kind of class (same with
"final"), I prefer "protected" usually. Shouldn't JavaClassCacheEntry be a
non static protected class ?

About webConfig, I'm not sure. A good way to do it would be to add calls to
a few intermediate empty methods in appropriate locations, rather than a
real refactoring.

Rémy

Re: open visibility of JavaClassCacheEntry

[Romain Manni-Bucau]

PS: alternative which can be saner would be to split webConfig() in a set
of small protected methods instead of a monolitic block, typically here a
processAnnotations(webXmls) not depending on internals can work

Romain Manni-Bucau
@rmannibucau  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | Book



Le jeu. 6 déc. 2018 à 11:51, Romain Manni-Bucau  a
écrit :

> Hi guys,
>
> can you make ContextConfig.JavaClassCacheEntry public please? Idea is to
> be able to override ContextConfig and potentially customize
> processAnnotations methods. Currently it is a pain and it is preventing to
> upgrade meecrowave and likely tomee to java11+jlink support
>
> Romain Manni-Bucau
> @rmannibucau  |  Blog
>  | Old Blog
>  | Github
>  | LinkedIn
>  | Book
> 
>

open visibility of JavaClassCacheEntry

[Romain Manni-Bucau]

Hi guys,

can you make ContextConfig.JavaClassCacheEntry public please? Idea is to be
able to override ContextConfig and potentially customize processAnnotations
methods. Currently it is a pain and it is preventing to upgrade meecrowave
and likely tomee to java11+jlink support

Romain Manni-Bucau
@rmannibucau  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | Book