Re: svn commit: r899053 - /ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java

[Adam Heath-2]

[hidden email] wrote:

> Author: adrianc
> Date: Thu Jan 14 03:54:23 2010
> New Revision: 899053
>
> URL: http://svn.apache.org/viewvc?rev=899053&view=rev
> Log:
> Better exception handling - suggested by Adam Heath.
>
> Modified:
>     ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java
>
> Modified: ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java
> URL: http://svn.apache.org/viewvc/ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java?rev=899053&r1=899052&r2=899053&view=diff
> ==============================================================================
> --- ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java (original)
> +++ ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java Thu Jan 14 03:54:23 2010
> @@ -20,6 +20,7 @@
>  
>  import static org.ofbiz.api.authorization.BasicPermissions.Access;
>  
> +import java.security.AccessControlException;
>  import java.util.List;
>  
>  import javolution.util.FastList;
> @@ -48,7 +49,9 @@
>              try {
>                  accessController.checkPermission(Access, artifactPath);
>                  resultList.add(webAppInfo);
> -            } catch (Exception e) {}
> +            } catch (AccessControlException e) {
> +                // This exception is expected - do nothing
> +            }
>              artifactPath.restoreState();

And what about the OutOfMemoryException that could be thrown by
resultList.add()?

Anytime you have a push/pop kind of pattern, you must use push before
a try, and pop inside the finally.

foo.push();
try {
        code();
} finally {
        foo.pop();
}

[Adrian Crum-2]

--- On Wed, 1/13/10, Adam Heath <[hidden email]> wrote:

> From: Adam Heath <[hidden email]>
> Subject: Re: svn commit: r899053 - /ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java
> To: [hidden email]
> Cc: [hidden email]
> Date: Wednesday, January 13, 2010, 8:17 PM
> [hidden email]
> wrote:
> > Author: adrianc
> > Date: Thu Jan 14 03:54:23 2010
> > New Revision: 899053
> >
> > URL: http://svn.apache.org/viewvc?rev=899053&view=rev
> > Log:
> > Better exception handling - suggested by Adam Heath.
> >
> > Modified:
> > 
>    ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java
> >
> > Modified:
> ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java
> > URL: http://svn.apache.org/viewvc/ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java?rev=899053&r1=899052&r2=899053&view=diff
> >
> ==============================================================================
> > ---
> ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java
> (original)
> > +++
> ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java
> Thu Jan 14 03:54:23 2010
> > @@ -20,6 +20,7 @@
> > 
> >  import static
> org.ofbiz.api.authorization.BasicPermissions.Access;
> > 
> > +import java.security.AccessControlException;
> >  import java.util.List;
> > 
> >  import javolution.util.FastList;
> > @@ -48,7 +49,9 @@
> >              try {
> >               
>   accessController.checkPermission(Access,
> artifactPath);
> >               
>   resultList.add(webAppInfo);
> > -            } catch
> (Exception e) {}
> > +            } catch
> (AccessControlException e) {
> > +             
>   // This exception is expected - do nothing
> > +            }
> >             
> artifactPath.restoreState();
>
> And what about the OutOfMemoryException that could be
> thrown by
> resultList.add()?
>
> Anytime you have a push/pop kind of pattern, you must use
> push before
> a try, and pop inside the finally.
>
> foo.push();
> try {
>     code();
> } finally {
>     foo.pop();
> }

I really appreciate you taking the time to review my code. In this case, I think you're making much ado about nothing. The object you are concerned about is on the stack, so if any exception is thrown it will be discarded.

[Adam Heath-2]

Adrian Crum wrote:

> --- On Wed, 1/13/10, Adam Heath <[hidden email]> wrote:
>
>> From: Adam Heath <[hidden email]>
>> Subject: Re: svn commit: r899053 - /ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java
>> To: [hidden email]
>> Cc: [hidden email]
>> Date: Wednesday, January 13, 2010, 8:17 PM
>> [hidden email]
>> wrote:
>>> Author: adrianc
>>> Date: Thu Jan 14 03:54:23 2010
>>> New Revision: 899053
>>>
>>> URL: http://svn.apache.org/viewvc?rev=899053&view=rev
>>> Log:
>>> Better exception handling - suggested by Adam Heath.
>>>
>>> Modified:
>>>  
>>    ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java
>>> Modified:
>> ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java
>>> URL: http://svn.apache.org/viewvc/ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java?rev=899053&r1=899052&r2=899053&view=diff
>>>
>> ==============================================================================
>>> ---
>> ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java
>> (original)
>>> +++
>> ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java
>> Thu Jan 14 03:54:23 2010
>>> @@ -20,6 +20,7 @@
>>>  
>>>   import static
>> org.ofbiz.api.authorization.BasicPermissions.Access;
>>>  
>>> +import java.security.AccessControlException;
>>>   import java.util.List;
>>>  
>>>   import javolution.util.FastList;
>>> @@ -48,7 +49,9 @@
>>>               try {
>>>                
>>   accessController.checkPermission(Access,
>> artifactPath);
>>>                
>>   resultList.add(webAppInfo);
>>> -            } catch
>> (Exception e) {}
>>> +            } catch
>> (AccessControlException e) {
>>> +            
>>   // This exception is expected - do nothing
>>> +            }
>>>              
>> artifactPath.restoreState();
>>
>> And what about the OutOfMemoryException that could be
>> thrown by
>> resultList.add()?
>>
>> Anytime you have a push/pop kind of pattern, you must use
>> push before
>> a try, and pop inside the finally.
>>
>> foo.push();
>> try {
>>     code();
>> } finally {
>>     foo.pop();
>> }
>
> I really appreciate you taking the time to review my code. In this case, I think you're making much ado about nothing. The object you are concerned about is on the stack, so if any exception is thrown it will be discarded.

Hmm.  I just switched my checkout to your branch.  I'm seeing some
things that need to be reviewed.  I'm going to need to make some time
for this.

Initially, ArtifactPath implements Iterator<String>.  I would have
done that as Iterable<String>, which would then allow ArtifactPath to
be used in for(type name: var) constructs.

Also, in the same class, getPathAsString has inconsistent used of
'this' when referencing the internal stringBuilder object.

Actually, what is the prefered way of making use of 'this'?  Should it
always be used on local method access?  local variable access?
Personally, I think it should *not* be used, as the compiler will
figure it out for you, and the computer should be used for what it is
good for, automation.

The static PATH_ROOT variable could get corrupted, if calling code
doesn't properly do a saveState/restoreState pair.  This is based on
my earlier emails in this thread.

Aie, this one is bad.  getPathAsString uses an instance variable,
stringBuilder, then manipulates it's state.  This could break in
multi-threaded situations, esp. when considering that PATH_ROOT is a
shared static.

Based on ContextUtil and ArtifactPath, it looks like
saveState/restoreState are *always* called.  This means that
ArtifactPath.stack will *always* be set to a FastList instance.  It
would be better to remove the null checks, and make stack a final
instance variable.

The array based ArtifactPath constructor should use ... instead of [],
to allow varargs.

It's generally bad form have an object take a parameter, then not
internalize said parameter.  Ie, the array constructor doesn't take
ownership of the pathElementArray, thereby allowing calling code to
manipulate it.  If this is by design, it should be listed as such in
the javadocs.

Does ArtifactPath support federation?  Ie, why not use jndi for this,
as it has path/name support stuff.

[Adrian Crum]

Adam Heath wrote:

> Hmm.  I just switched my checkout to your branch.  I'm seeing some
> things that need to be reviewed.  I'm going to need to make some time
> for this.
>
> Initially, ArtifactPath implements Iterator<String>.  I would have
> done that as Iterable<String>, which would then allow ArtifactPath to
> be used in for(type name: var) constructs.
>
> Also, in the same class, getPathAsString has inconsistent used of
> 'this' when referencing the internal stringBuilder object.
>
> Actually, what is the prefered way of making use of 'this'?  Should it
> always be used on local method access?  local variable access?
> Personally, I think it should *not* be used, as the compiler will
> figure it out for you, and the computer should be used for what it is
> good for, automation.
>
> The static PATH_ROOT variable could get corrupted, if calling code
> doesn't properly do a saveState/restoreState pair.  This is based on
> my earlier emails in this thread.
>
> Aie, this one is bad.  getPathAsString uses an instance variable,
> stringBuilder, then manipulates it's state.  This could break in
> multi-threaded situations, esp. when considering that PATH_ROOT is a
> shared static.
>
> Based on ContextUtil and ArtifactPath, it looks like
> saveState/restoreState are *always* called.  This means that
> ArtifactPath.stack will *always* be set to a FastList instance.  It
> would be better to remove the null checks, and make stack a final
> instance variable.
>
> The array based ArtifactPath constructor should use ... instead of [],
> to allow varargs.
>
> It's generally bad form have an object take a parameter, then not
> internalize said parameter.  Ie, the array constructor doesn't take
> ownership of the pathElementArray, thereby allowing calling code to
> manipulate it.  If this is by design, it should be listed as such in
> the javadocs.
>
> Does ArtifactPath support federation?  Ie, why not use jndi for this,
> as it has path/name support stuff.

Those are good suggestions - thank you for taking the time to look
things over. Good catch on the static PATH_ROOT - I hadn't thought of that.

I need to put this on the shelf for a while and get back to work on the
trunk. If anyone wants to make changes to the branch, they are welcome
to do so.

-Adrian

[Adrian Crum]

Adam Heath wrote:
> It's generally bad form have an object take a parameter, then not
> internalize said parameter.  Ie, the array constructor doesn't take
> ownership of the pathElementArray, thereby allowing calling code to
> manipulate it.  If this is by design, it should be listed as such in
> the javadocs.

That was by design. I didn't put a great deal of thought into that class
initially - it started out as a convenience. As development progressed,
it evolved. Writing JavaDocs is a LONG way off.

A lot of the improvements you suggested (and are committing) I would
have gotten around to eventually. My focus was to get it operational so
it can be evaluated.

By evaluated I mean on a higher level than what you're doing here. I
need the *concept* evaluated and commented on. Will this approach to
security work? Can we agree on its advantages? Try converting a
component over to the new security - was it easier or harder than
expected? How will this fit in with your projects? How can it be
expanded/improved/made easier?

-Adrian

[Adam Heath-2]

Adrian Crum wrote:

> Adam Heath wrote:
>> It's generally bad form have an object take a parameter, then not
>> internalize said parameter.  Ie, the array constructor doesn't take
>> ownership of the pathElementArray, thereby allowing calling code to
>> manipulate it.  If this is by design, it should be listed as such in
>> the javadocs.
>
> That was by design. I didn't put a great deal of thought into that class
> initially - it started out as a convenience. As development progressed,
> it evolved. Writing JavaDocs is a LONG way off.
>
> A lot of the improvements you suggested (and are committing) I would
> have gotten around to eventually. My focus was to get it operational so
> it can be evaluated.
>
> By evaluated I mean on a higher level than what you're doing here. I
> need the *concept* evaluated and commented on. Will this approach to
> security work? Can we agree on its advantages? Try converting a
> component over to the new security - was it easier or harder than
> expected? How will this fit in with your projects? How can it be
> expanded/improved/made easier?

To do that kind of high-level evaluation, I need quick scans, and let
my gut respond.  It has a hard time doing that when there are littly
nit-picky style issues.

Plus, going thru all files like I have done allows me to see all the
files quickly, and try to get a feel for them.  I haven't read the
design docs, haven't read the readme file, just trying to read the
code to see if it makes sense.

Initially, I like the idea of what is occuring here.  There is more
than I thought, based on just reading the list.  Some of the things
you are doing we've wanted to do with webslinger, so it's good that
there is an implementation in ofbiz now.

I'll continue to review, but it'll be a bit before I can actually sit
down for an extended period.

[Adam Heath-2]

Adam Heath wrote:
> Adrian Crum wrote:
>> By evaluated I mean on a higher level than what you're doing here. I
>> need the *concept* evaluated and commented on. Will this approach to
>> security work? Can we agree on its advantages? Try converting a
>> component over to the new security - was it easier or harder than
>> expected? How will this fit in with your projects? How can it be
>> expanded/improved/made easier?


So, I see a general pattern in these classes, separate push()/pop()
methods, that have to be called correctly.  Due to developer error, or
runtime error, it's possible that a pop() might not be called.

I would instead prefer to see this:

T result = Controller.runWith(data, new Callable<T>() {
    public T call() throws Exception {
        // code
        return null;
    }
});

runWith can then encapsulate the correct try/catch/finally/push/pop
handling.

ps: OfbizSecurityTransform.execute() should call pushExecutionArtifact
outside of the try, not inside.

[Adrian Crum]

Adam Heath wrote:
> ps: OfbizSecurityTransform.execute() should call pushExecutionArtifact
> outside of the try, not inside.

Good point. That will need to be fixed in a number of places.

[Adam Heath-2]

Adam Heath wrote:
> T result = Controller.runWith(data, new Callable<T>() {
>     public T call() throws Exception {
>         // code
>         return null;
>     }
> });

I've actually attempted this, and while I think the implementation of
this pattern is simple, actually *using* it in higher-level code ends
up making things rather verbose.

/me goes to think more

[Adrian Crum]

Adam Heath wrote:

> Adam Heath wrote:
>> T result = Controller.runWith(data, new Callable<T>() {
>>     public T call() throws Exception {
>>         // code
>>         return null;
>>     }
>> });
>
> I've actually attempted this, and while I think the implementation of
> this pattern is simple, actually *using* it in higher-level code ends
> up making things rather verbose.
>
> /me goes to think more

I like the idea of encapsulating it all, but it seems to me at first
glance that it will require a lot of code rewriting. I was trying to
"inject" the new design into existing code without altering the existing
code. New code could certainly follow a better pattern.

[Adam Heath-2]

Adrian Crum wrote:

> Adam Heath wrote:
>> Adam Heath wrote:
>>> T result = Controller.runWith(data, new Callable<T>() {
>>>     public T call() throws Exception {
>>>         // code
>>>         return null;
>>>     }
>>> });
>>
>> I've actually attempted this, and while I think the implementation of
>> this pattern is simple, actually *using* it in higher-level code ends
>> up making things rather verbose.
>>
>> /me goes to think more
>
> I like the idea of encapsulating it all, but it seems to me at first
> glance that it will require a lot of code rewriting. I was trying to
> "inject" the new design into existing code without altering the existing
> code. New code could certainly follow a better pattern.
>

Why wasn't java.security.AccessController(and friends) used for this?

[Adrian Crum]

Adam Heath wrote:

> Adrian Crum wrote:
>> Adam Heath wrote:
>>> Adam Heath wrote:
>>>> T result = Controller.runWith(data, new Callable<T>() {
>>>>     public T call() throws Exception {
>>>>         // code
>>>>         return null;
>>>>     }
>>>> });
>>> I've actually attempted this, and while I think the implementation of
>>> this pattern is simple, actually *using* it in higher-level code ends
>>> up making things rather verbose.
>>>
>>> /me goes to think more
>> I like the idea of encapsulating it all, but it seems to me at first
>> glance that it will require a lot of code rewriting. I was trying to
>> "inject" the new design into existing code without altering the existing
>> code. New code could certainly follow a better pattern.
>>
>
> Why wasn't java.security.AccessController(and friends) used for this?

I don't know. I had a good reason at the time, but I forgot what it was.
Since the API is the same, I'm sure we could try changing it.

[Adrian Crum-2]

--- On Thu, 1/14/10, Adrian Crum <[hidden email]> wrote:

> From: Adrian Crum <[hidden email]>
> Subject: Re: svn commit: r899053 - /ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java
> To: [hidden email]
> Date: Thursday, January 14, 2010, 4:23 PM
> Adam Heath wrote:
> > Adrian Crum wrote:
> >> Adam Heath wrote:
> >>> Adam Heath wrote:
> >>>> T result = Controller.runWith(data, new
> Callable<T>() {
> >>>>     public T call()
> throws Exception {
> >>>>         //
> code
> >>>>     
>    return null;
> >>>>     }
> >>>> });
> >>> I've actually attempted this, and while I
> think the implementation of
> >>> this pattern is simple, actually *using* it in
> higher-level code ends
> >>> up making things rather verbose.
> >>>
> >>> /me goes to think more
> >> I like the idea of encapsulating it all, but it
> seems to me at first
> >> glance that it will require a lot of code
> rewriting. I was trying to
> >> "inject" the new design into existing code without
> altering the existing
> >> code. New code could certainly follow a better
> pattern.
> >>
> >
> > Why wasn't java.security.AccessController(and friends)
> used for this?
>
> I don't know. I had a good reason at the time, but I forgot
> what it was.
> Since the API is the same, I'm sure we could try changing
> it.

I just remembered what the reason was. During the mailing list discussion, it was mentioned that we should create an API based on a set of interfaces, so the OFBiz implementation could be replaced by another one. The goal is to have an out of the box implementation based on the entity engine, but other implementations could be developed based on third party libraries or something.

[Adrian Crum-2]

--- On Thu, 1/14/10, Adam Heath <[hidden email]> wrote:

> From: Adam Heath <[hidden email]>
> Subject: Re: svn commit: r899053 - /ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java
> To: [hidden email]
> Date: Thursday, January 14, 2010, 3:54 PM
> Adam Heath wrote:
> > T result = Controller.runWith(data, new
> Callable<T>() {
> >     public T call() throws
> Exception {
> >         // code
> >         return null;
> >     }
> > });
>
> I've actually attempted this, and while I think the
> implementation of
> this pattern is simple, actually *using* it in higher-level
> code ends
> up making things rather verbose.
>
> /me goes to think more

Just wondering - why would using it in higher level code be necessary? The higher level code should be using the security-aware artifacts, and not try to be one.

[Adrian Crum]

Adam Heath wrote:

> Adam Heath wrote:
>> T result = Controller.runWith(data, new Callable<T>() {
>>     public T call() throws Exception {
>>         // code
>>         return null;
>>     }
>> });
>
> I've actually attempted this, and while I think the implementation of
> this pattern is simple, actually *using* it in higher-level code ends
> up making things rather verbose.

I just added your idea to the branch, and it *is* simple. Let's say we
want to make incoming requests a security-aware artifact:

org.ofbiz.webapp.control.ControlServlet

public void doGet(HttpServletRequest request, HttpServletResponse
response) throws ServletException, IOException {
     try {
         ThreadContext.runExecutionArtifact(new RequestArtifact(request,
response));
     } catch (Throwable t) {
         // Do something with t
     }
}

public class RequestArtifact implements ExecutionArtifact {
...
}

[Adam Heath-2]

Adrian Crum wrote:

> Adam Heath wrote:
>> Adam Heath wrote:
>>> T result = Controller.runWith(data, new Callable<T>() {
>>>     public T call() throws Exception {
>>>         // code
>>>         return null;
>>>     }
>>> });
>>
>> I've actually attempted this, and while I think the implementation of
>> this pattern is simple, actually *using* it in higher-level code ends
>> up making things rather verbose.
>
> I just added your idea to the branch, and it *is* simple. Let's say we
> want to make incoming requests a security-aware artifact:
>
> org.ofbiz.webapp.control.ControlServlet
>
> public void doGet(HttpServletRequest request, HttpServletResponse
> response) throws ServletException, IOException {
>     try {
>         ThreadContext.runExecutionArtifact(new RequestArtifact(request,
> response));
>     } catch (Throwable t) {
>         // Do something with t
>     }
> }

If you were *really* hot stuff, you'd use APT, annotations, and
directly modify the code at compile time.

@ThreadContext(RequestArtifact.class)
public void doGet(HttpServletRequest request, HttpServletResponse
response) {
    if (request.getAttribute()) {
        // ...
    }
}

[Adrian Crum]

Adam Heath wrote:

> Adrian Crum wrote:
>> Adam Heath wrote:
>>> Adam Heath wrote:
>>>> T result = Controller.runWith(data, new Callable<T>() {
>>>>     public T call() throws Exception {
>>>>         // code
>>>>         return null;
>>>>     }
>>>> });
>>> I've actually attempted this, and while I think the implementation of
>>> this pattern is simple, actually *using* it in higher-level code ends
>>> up making things rather verbose.
>> I just added your idea to the branch, and it *is* simple. Let's say we
>> want to make incoming requests a security-aware artifact:
>>
>> org.ofbiz.webapp.control.ControlServlet
>>
>> public void doGet(HttpServletRequest request, HttpServletResponse
>> response) throws ServletException, IOException {
>>     try {
>>         ThreadContext.runExecutionArtifact(new RequestArtifact(request,
>> response));
>>     } catch (Throwable t) {
>>         // Do something with t
>>     }
>> }
>
> If you were *really* hot stuff, you'd use APT, annotations, and
> directly modify the code at compile time.
>
> @ThreadContext(RequestArtifact.class)
> public void doGet(HttpServletRequest request, HttpServletResponse
> response) {
>     if (request.getAttribute()) {
>         // ...
>     }
> }

Alas, I'm only semi-hot stuff.

[David E. Jones-2]

On Jan 15, 2010, at 2:12 PM, Adam Heath wrote:

> Adrian Crum wrote:
>> Adam Heath wrote:
>>> Adam Heath wrote:
>>>> T result = Controller.runWith(data, new Callable<T>() {
>>>>    public T call() throws Exception {
>>>>        // code
>>>>        return null;
>>>>    }
>>>> });
>>>
>>> I've actually attempted this, and while I think the implementation of
>>> this pattern is simple, actually *using* it in higher-level code ends
>>> up making things rather verbose.
>>
>> I just added your idea to the branch, and it *is* simple. Let's say we
>> want to make incoming requests a security-aware artifact:
>>
>> org.ofbiz.webapp.control.ControlServlet
>>
>> public void doGet(HttpServletRequest request, HttpServletResponse
>> response) throws ServletException, IOException {
>>    try {
>>        ThreadContext.runExecutionArtifact(new RequestArtifact(request,
>> response));
>>    } catch (Throwable t) {
>>        // Do something with t
>>    }
>> }
>
> If you were *really* hot stuff, you'd use APT, annotations, and
> directly modify the code at compile time.
>
> @ThreadContext(RequestArtifact.class)
> public void doGet(HttpServletRequest request, HttpServletResponse
> response) {
>    if (request.getAttribute()) {
>        // ...
>    }
> }

Yes, please... let's make this as obfuscated and impossible to maintain as possible.

This may be a community driven project, but job security should still be our #1 priority. On the other hand, I think it was Adam who said that after a couple of years he couldn't even follow his own code in parts of the Entity Engine (yes, stuff I've been tempted to rewrite since it's so tedious to trace through, though I think the ShoppingCart and related objects are still worse!).

-David

[Adam Heath-2]

David E Jones wrote:

> On Jan 15, 2010, at 2:12 PM, Adam Heath wrote:
>
>> Adrian Crum wrote:
>>> Adam Heath wrote:
>>>> Adam Heath wrote:
>>>>> T result = Controller.runWith(data, new Callable<T>() {
>>>>>    public T call() throws Exception {
>>>>>        // code
>>>>>        return null;
>>>>>    }
>>>>> });
>>>> I've actually attempted this, and while I think the implementation of
>>>> this pattern is simple, actually *using* it in higher-level code ends
>>>> up making things rather verbose.
>>> I just added your idea to the branch, and it *is* simple. Let's say we
>>> want to make incoming requests a security-aware artifact:
>>>
>>> org.ofbiz.webapp.control.ControlServlet
>>>
>>> public void doGet(HttpServletRequest request, HttpServletResponse
>>> response) throws ServletException, IOException {
>>>    try {
>>>        ThreadContext.runExecutionArtifact(new RequestArtifact(request,
>>> response));
>>>    } catch (Throwable t) {
>>>        // Do something with t
>>>    }
>>> }
>> If you were *really* hot stuff, you'd use APT, annotations, and
>> directly modify the code at compile time.
>>
>> @ThreadContext(RequestArtifact.class)
>> public void doGet(HttpServletRequest request, HttpServletResponse
>> response) {
>>    if (request.getAttribute()) {
>>        // ...
>>    }
>> }
>
> Yes, please... let's make this as obfuscated and impossible to maintain as possible.
>
> This may be a community driven project, but job security should still be our #1 priority. On the other hand, I think it was Adam who said that after a couple of years he couldn't even follow his own code in parts of the Entity Engine (yes, stuff I've been tempted to rewrite since it's so tedious to trace through, though I think the ShoppingCart and related objects are still worse!).

Yeah, that was me, and it is the caching stuff.  I agree about the
shopping cart and order read header too.

In all honesty, I would use dynamic ecas to do the caching stuff now.

[David E. Jones-2]

On Jan 15, 2010, at 2:33 PM, Adam Heath wrote:

> David E Jones wrote:
>> On Jan 15, 2010, at 2:12 PM, Adam Heath wrote:
>>
>>> Adrian Crum wrote:
>>>> Adam Heath wrote:
>>>>> Adam Heath wrote:
>>>>>> T result = Controller.runWith(data, new Callable<T>() {
>>>>>>   public T call() throws Exception {
>>>>>>       // code
>>>>>>       return null;
>>>>>>   }
>>>>>> });
>>>>> I've actually attempted this, and while I think the implementation of
>>>>> this pattern is simple, actually *using* it in higher-level code ends
>>>>> up making things rather verbose.
>>>> I just added your idea to the branch, and it *is* simple. Let's say we
>>>> want to make incoming requests a security-aware artifact:
>>>>
>>>> org.ofbiz.webapp.control.ControlServlet
>>>>
>>>> public void doGet(HttpServletRequest request, HttpServletResponse
>>>> response) throws ServletException, IOException {
>>>>   try {
>>>>       ThreadContext.runExecutionArtifact(new RequestArtifact(request,
>>>> response));
>>>>   } catch (Throwable t) {
>>>>       // Do something with t
>>>>   }
>>>> }
>>> If you were *really* hot stuff, you'd use APT, annotations, and
>>> directly modify the code at compile time.
>>>
>>> @ThreadContext(RequestArtifact.class)
>>> public void doGet(HttpServletRequest request, HttpServletResponse
>>> response) {
>>>   if (request.getAttribute()) {
>>>       // ...
>>>   }
>>> }
>>
>> Yes, please... let's make this as obfuscated and impossible to maintain as possible.
>>
>> This may be a community driven project, but job security should still be our #1 priority. On the other hand, I think it was Adam who said that after a couple of years he couldn't even follow his own code in parts of the Entity Engine (yes, stuff I've been tempted to rewrite since it's so tedious to trace through, though I think the ShoppingCart and related objects are still worse!).
>
> Yeah, that was me, and it is the caching stuff.  I agree about the
> shopping cart and order read header too.
>
> In all honesty, I would use dynamic ecas to do the caching stuff now.

Do you mean like Entity ECAs? I'm not sure how that would work, but it would be an interesting way to externalize things.

As a side note that is somewhat related, I think reverse associations/injections in code (like aspect oriented stuff) is another great tool of obfuscation that if skillfully applied can not only grow the initial effort required for development, but can also explode the maintenance effort required. That's why so many of us consultants like it so much. ;)

-David