Adam,
You I must have a different definition of robust. Two of the MANY
examples that illustrate my point is as follows:
1. There is no error checking on disableConfiguratorServices method and,
the implementation as it stand now, could force us to break our contract
with the configurators. Furthermore, it's not always obvious that
something wrong was done. For instance, the following code will not
generate an error:
FacesContectFactory.getFacesContext(context, request, response, lifecycle);
...
Configurator.disableConfiguratorServices();
This will not generate an error yet cleanup (endRequest) will not occur
even though beginRequest WAS called (when the FacesContextFactory was
created OR when the filter is run). getExternalContext MAY or MAY NOT
be wrapped depending on whether or not an external context was retrieved
before the disable. The basically leaves the status of externalContext
in an unknown state and will not execute the end Request. You
eliminated the "reflection" in my latest patch by getting rid of this
check. In a public "rhobust" API, this really needs to be enforced. It
USED to call an IllegalStateException so it didn't necessarily have to
be caught because it should be fairly obvious at the time someone writes
their code.
2. You got rid of reflection in the public Configurator class by using
Trinidad's Service Loader. This class not ONLY uses reflection, but it
also reads a bunch of files and returns an "array" of initialized
services. Do we really need to open some file objects and initialize an
extra array for no reason? Now it's an interesting idea to be able to
replace the global configurator, but first off that wasn't a requirement
that I was coding for and second off, we cannot guarantee the "order"
that the services files are loaded in. Again this leaves us in an
unknown state if we are trying to override the default functionality and
is simply using a sledge hammer to drive a nail if we aren't.
You were able to eliminate a lot of the casting by not exposing the is
initialized method on the global configurator by moving the
initialization code inside of "getInstance" method. I kept this
separate so that I could adjust to JSR-301 as I need to. Butt the API
is now locked in to perform initialization at that stage. This is
something that very well could become a problem. And I haven't even
taken a look at how you handled the isInRequest functionality that I
need to maintain the proper flexibility in the portal case.
Still, I really don't have time to argue these points. I'll just have
to generate Jira tickets as problems come up and we'll have to change
the API's if we need to change them. All I really have to say about the
subject is allowing the GlobalConfigurator implementation handles all
these issues for free. And using my latest checkins handles most of
them, just not as cleanly. Certainly. though, a lot more cleanly then
using the "Service Loader".
You said that you didn't know if these changes worked in a portal
environment, and I'm not real sure I can confirm that within the next
week or so. But checking in changes to a "portal compatibility" branch
without being able to check them in a Proof of Concept (POC) is really
damaging to the project being that Portal compatibility is what this
project is all about. I was able to test my stuff as a POC using the
Oracle Portal and Bridge, and I'm also aware of the changes that need to
be made in the MyFaces Bridge to support these cases. Many of these
issues ARE going to be addressed in JSR-301 but my hope it to have the
myfaces bridge up to par before that time.
My final, and I mean final, comment on this is that it is important to
me to have error checking on disableConfiguratorServices. That's from a
robust API perspective. From a Portal standpoint, it is far more
important for me to have control over the initialization logic and to
error out of getInstance when an instance is not available (or return
null, I don't care which) then it is for me to have an exposed
"getInstance()" in the API and for the thing to return a dummy
configurator. Exposing the getInstance made sense when we had the
ability to tack additional non-static API's onto the
GlobalConfigurator. Since we don't, I can't see ANY reason why someone
would need this API outside of Trinidad. The object is simply "not
useful" outside of this codebase anymore. Plus it keeps us flexible to
add an API later if we need one whereas this current implementation
LOCKS us in to providing a plain run-of-the-mill configurator. Lastly,
for GOD sake, don't use the service loading system to load this class.
The Service loading system is great for making extensions to Trinidad,
but it has limitations which stem from the fact that order within these
services is not guaranteed. For now I say we simply have the
GlobalConfiguratorImpl (in the Impl package) be referenced directly if
we need to, and if we have the need to add additional
globalConfigurators in the future, we can come up with an API that makes
sense. This one makes no sense at all. Essentially I outlined for you
the things in my last checkin. You're latest checkin' will lock us into
unwanted implementations, implementations that make NO sense what so
ever, and implementations that are not robust.
As I stated before, though, I don't really have time to argue with you
anymore. I have some other projects that I need to work on at the
moment. So if you're happy with the API, call a vote and let's get this
moved in so I can move forward and we can hopefully have trinidad
working inside of a MyFaces Portal environment sometime this century.
Scott O'Bryan
Adam Winer wrote:
On 12/21/06, Scott O'Bryan <[EMAIL PROTECTED]> wrote:
Adam,
Well, you basically implemented one of the solutions I said I didn't
like earlier, but oh well. And there are a number of places you need to
cast. So the concerns are still valid.
Well, I don't get that claim, as I didn't add a single cast in my
patch at all, and removed references to the impl class, so I can't
imagine
how I've made things worse. And I didn't really see any casts that were
already there.
The one question I do have is why does getInstance take in an
ExternalContext? I'm assuming it's because your executing the init
inside of the getInstance object and I'm not sure this is the correct
place for this. My hope in all of this was to be able to explicitly
handle initialization of this object. Plus, nine times out of ten, the
ExternalContext object is ignored in the call to this method.
A create + initialize construct is more robust than a create-and-hope-
it-gets-initialized-by-someone-else construct.
-- Adam
Either way, don't see as I really have the time to argue. So is it
acceptable to everyone?
Also, there is no
Adam Winer wrote:
> Scott,
>
> OK, well, I just went ahead and implemented what I was trying
> to say, to see if I'd run into the problems you're describing. I
> didn't...
> (It's possible I've broken something in portlet land - I only tested
> the changes in a servlet environment.)
>
> On 12/21/06, Scott O'Bryan <[EMAIL PROTECTED]> wrote:
>> Adding getInstance() to the configurator will either force us to cast
in
>> a bunch of different places
>
> No, it doesn't. No casts were required at all, much
> less in a bunch of places, and most of the code now
> doesn't even have references to the impl class at all.
>
>> or to expose the GlobalConfiguratorImpl's
>> api to the rest of the world (which I don't want to do because
they are
>> applicable ONLY to global configurator. And it won't lock us into an
>> API we may have to expand later.
>>
>> As for simply putting the Boolean on the request map, either I'll
have
>> to make a protected constant on the Configurator interface (which has
no
>> bearing on any of the configurators except the global configurator so
it
>> shouldn't go into the ancestor) or I add a porotected (isDisabled)
>> method (which, again, is only applicable to the
GlobalConfiguratorImpl
>> and therfore shouldn't do into it's Ancestor).
>
> See the code checked in, which does not suffer these probems.
>
>> I've never been one to include a feature into an interface or a class
>> that is applicable in only one instance of that class because it
>> violated basics OO design principals. The only other option I see
here
>> is to define the constant used to store the boolean in BOTH
classes and
>> hope they remain in sync, but I've never been a big fan of that
either.
>
> Again, see the code checked in.
>
> -- Adam
>
