myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Struberg <strub...@yahoo.de>
Subject Re: [DISCUSS] why do we overcomplicate our code soooo much?
Date Fri, 28 Oct 2011 20:07:15 GMT
Exactly there was a vote. Please go on and read the threads you posted again!


a.) the vote you linked to has nothing to do with what you did implement and b) exactly in
this thread you can see that already 5 people mentioned 15 times that you are really wrong
and that they do not share your opinion in this points! You even force people to fork off
the project because you cannot accept a different opinion and randomly revert the work done
by others.


Leo, I really like the work you do on myfaces-core, but please let others do their projects
as well. This was Jakobs project and you perfectly screwed him off. I originally didn't pay
too much attention on the technical side, thus I didn't say much - but after having a detailed
look at the problem (because I need to make use of it), I can only say that the changes are
far from good.


> That's something new. That wiki page has date from October 13.
The wiki page of the project is actually older. Plus most of this was also documented in the
code you deleted.

Another option for getting this info would have been ASKING. Or just listening to the explanations
you got (for example on the threads you linked).

> but that does
> not means that the final code should solve ONLY this case.

Actually yes. The project Jakob started had the pure intention to just remove the need that
JSF resources cannot get cached, thus they hit the server with each and every page reload.
That is really the ONLY problem this code should solve! If there are some neat little side
goodies than I'm fine with that as well. But please don't introduce new features which blow
up the code to 10-times the LOC.



> There was an original list and then
> we try to add some features.
where did _we_ try to add some features? JIRA number? 


>>  * Prevent use #{resource['...']} on css files, using prefix mapping
>>  a.) there is no JIRA for it.
> 
> If that comes from the spec, isn't suppose a "extended" resource
> handler should implement it? I created an issue to fix a problem
> related to this one:
> 
> https://issues.apache.org/jira/browse/MFCOMMONS-38
Again: JIRA number? because MFCOMMONS-38 has nothing to do with it due to the fact that the
logic is the wrong way around...

> I don't think that's complicated. There is one custom InputStream that 
> checks
> the output for #{ and if it found something, it try to evaluate, if is success
> it output the result, if it is not, it continues.
We can get rid of all the ValueExpressionFilterInputStream and associated other sources.
It really doesn't matter if the EL in the css is valid or not. Not a bit, njente, nada...
Thats just useless code... If there is any #{ (or "#{resource" would need to think about it)
in the code, then it's just not cacheable.
What problem did you like to solve by evaluating the ELs?

> Maybe you're saying web.xml parsing. That's for prefix mapping automatic
> detection.
Oki, then I vote for manually configuring the prefix. 
a.) parsing web.xml is not easy and error prone. because the META-INF/web.xml is NOT what
finally will get used! It will be merged with the server provided web.xml. It might get changed
by web-fragments in Servlet-3.0 environments, etc.
b.) by configuring an absolute address for a pattern, you will also get your (another 10++
classes heavy) solution for configuring 'external' resources for free. E.g. if you like to
use 1 shared resource address for multiple webapps.


In general: I think the whole configuration must not be done in web.xml or faces-config.xml.
It really should be done in an own config file. And it should support a mechanism to exchange
the config programmatically or via ProjectStage. This is needed becaues you might like to
use different configs in production.


>>  I agree that it could be usable to have this configurable - but why via EL? 
> Just too complicated.
>> 
> 
> I don't think so. In faces-config.xml files we use EL for navigation rules.
> Suggestions are welcome.
This was not really an answer. Please bring arguments. Why do we need dynamic configuration
for static information?


>>  resourceName of our resourcehandler != resourceName of JSF resource 
> handling!
>> 
> 
> I know, but it is a terrible trick. I have told this before, but that
> strategy leads
> with some ambiguities.

Actually it's not a 'trick' but it's a really well working pattern. We expand all information
we have to get a new restful URI.


> But it is evidence that the code has design mistakes, and is not using
> the API as it is supposed.
Actually I think the spec has design mistakes in this area. We try to solve this in a portable,
but not too tightly integrated fashion.

> http://{server}[:port]/{appPath}/javax.faces.resource/de_AT/mydir/myresource.js
> 
> The algorithm will detect de as a locale prefix, mydir as a library and
> myresource.js as a resource name, but that's wrong because the
> resource name is de/mydir/myresource.js. ...."
Thats exactly why this doesn't work. And there is a really easy solution for it by using reproducible
restful URLs. 
But first we would need to drop all the stuff we don't need. And I honestly fear this is more
work than just starting from scratch again and consider the current trunk as 'prototype' which
just didn't work out.


LieGrue,
strub


----- Original Message -----
> From: Leonardo Uribe <lu4242@gmail.com>
> To: MyFaces Development <dev@myfaces.apache.org>; Mark Struberg <struberg@yahoo.de>
> Cc: 
> Sent: Friday, October 28, 2011 7:13 PM
> Subject: Re: [DISCUSS] why do we overcomplicate our code soooo much?
> 
> Hi Mark
> 
> 2011/10/28 Mark Struberg <struberg@yahoo.de>:
>>  Leo,
>> 
>> 
>>>  Is like the blind that try to see the elephant
>> 
>> 
>>  Thanks for the flowers, but actually we USE this already in practice! And 
> we cannot use
>>  mf-commons-resourcehandler because it crashes with multiple servlets.
>>  Thus I was looking whether we can fix mf-commons-resourcehandler, or we
>>  shall go and improve Jakobs version.
>> 
>>  Also I don't see why Jakobs @author tags got removed from sources which
>>  obviously originated from him (although now renamed and changed). Also
>>  SVN is fuuu up most probably because of some svn mv. I cannot just
>>  revert the code locally to see what was there originally (although svn
>>  log shows the changes).
>> 
> 
> Checking the svn log the code committed by jakob was removed in
> rev 1170571 and rev 1170292 with date 14 September 2011, which
> was just a month ago. Why? because I wanted to keep it on the svn
> as reference, and the idea was discuss about both strategies.
> Check these links to see the files:
> 
> http://svn.apache.org/viewvc/myfaces/commons/branches/jsf_20/myfaces-commons-resourcehandler/src/main/java/org/apache/myfaces/commons/resourcehandler/AdvancedResource.java?revision=1095901&view=markup&pathrev=1147474
> http://svn.apache.org/viewvc/myfaces/commons/branches/jsf_20/myfaces-commons-resourcehandler/src/main/java/org/apache/myfaces/commons/resourcehandler/AdvancedResourceHandler.java?revision=1095901&view=markup&pathrev=1147474
> 
> You can svn update the code to an specified revision to see
> which code was on the svn in that time. You don't need to revert it.
> 
> Just as reference below there are the original mails where all this
> discussion started:
> 
> http://markmail.org/message/glr356g45uta5yys?q=Advanced+Resource+Handler
> http://markmail.org/message/zuydfi2kug3ns52w?q=vote+how+to+list:org%2Eapache%2Emyfaces%2Edev+order:date-backward&page=8
> 
> So, this discussion dates from February, the alternate proposal
> was committed on Jun 13, but the other code was not removed,
> instead a mail was sent to start a discussion about how this
> module should looks like.
> 
>> 
>>  Let's go through the list:
>> 
>>>  I think you have the wrong impression, basically because you 
> haven't
>>>  tried to solve each one of the problems that the code try to solve.
>>  Actually I know that there are a few things to do, but the 'new' 
> code solves
>>  problems which just do not exist! It's now completely over-engineered
>>  imo - more details later on.
>> 
>>>  1. Which features should be included:
>> 
>>  For the requirements please read
>> 
> http://code.google.com/a/apache-extras.org/p/relative-resource-handler/wiki/Requirements
>> 
> 
> That's something new. That wiki page has date from October 13. So
> I suppose it is a "refined" list from the original intention sent on
> February 23
> to this list.
> 
>>  Just to explain this again: We _only_ really need to handle
>>  javax.faces.application.Resource#getRequestPath()
>> 
>>  and the special handling is only for 'new resources' which 
> don't contain EL or any other dynamic stuff! Of course the structure and 
> fallbacks still might be dynamic - but reproducable (as example: if no okButton 
> for language=cn exists the resource with language=en will get served).
>> 
>> 
>>  In case of such a ‘new resource’ we will expand ALL variable parameters
>>  (language, version, useragent - this list should finally be
>>  user-extendable) to one bit REST URL, always containing  ALL parameters
>>  (and if it’s only the default value, e.g _en_ for language ‘english’ as
>>  default).
>> 
>>  This URL doesn’t necessarily need to be treated via a JSF resource request 
> - actually it’s something COMPLETELY different! This can also be a pure
>>  Servlet! It’s really completely split from JSF - there is no info needed 
> from JSF anymore, because ALL the info is in the restful resource URL
>>  itself! There is no ‘?ln=css’ or whatever in those URLs, just pure static 
> info packed into the restful URL.
>> 
>>  The only thing we need to make sure is that _other_ resources will still 
> get served via the standard JSF resource mechanism.
>>  But really, we don’t even need to intercept the JSF resource GET requests.
>>  Because the ‘new’ resources might not even be served as such...
>> 
> 
> Ok, that's new information. Thanks for the clarifications about your
> needs and the intention. I see this more as a "use case" than
> myfaces-commons-resourcehandler should solve, but that does
> not means that the final code should solve ONLY this case.
> 
>> 
>>  We imo don’t need (points from your list):
>> 
> 
> It is not my list, that should be clear. There was an original list and then
> we try to add some features.
> 
>>  * gzip stuff: can be applied via Filter or httpd mod in front as already
>>  explained. There are perfect solutions on the market for this already.
>> 
> 
> After thinking about it, the code that handle gzip stuff has these
> advantages:
> 
> - No additional buffer, reducing memory usage.
> - Cache files on temp dir using JSF 2 Resource Handler logic.
> 
> but its disadvantages
> 
> - Hinder Resource Handler API abstractions
> - No pluggable.
> - Better known ways.
> - Lack of agent detection and configuration using other params
> 
> At first it sounded like a good idea but after implement it and see the
> result, it is clear remove gzip stuff is the right choice.
> 
>>  * caching: don’t needed because the new solution will enable caching on the 
> CLIENT!
>>  If you really need server side caching, then any users can enable a
>>  caching Filter or http mod_cache or mod_file_cache, because the URLs are 
> fully REST! One URL will _always_ return the same binary!
>> 
> 
> caching was for gzip resources, so yes, let's remove that too.
> 
>>  * Prevent use #{resource['...']} on css files, using prefix mapping
>>  a.) there is no JIRA for it.
> 
> If that comes from the spec, isn't suppose a "extended" resource
> handler should implement it? I created an issue to fix a problem
> related to this one:
> 
> https://issues.apache.org/jira/browse/MFCOMMONS-38
> 
> but the default behavior was inherited from the default algorithm.
> 
>>  b.) Actually (given the expense we have for doing this in the current code
>>  base + the expense this costs at runtime atm) I’d favour to not do the
>>  check at runtime.
>>  Why? Because every developer will quickly see that his resources will be
>>  broken anyway. And for resources provided by some 3-rd party, those
>>  can/should get excluded in the config.
>>  We could probably just log a warning if css contains “#{“ in
>>  ProjectStage.Development to make excluding easier. This is just 10 lines of 
> code and we are done.
>> 
>>  Otoh I’m not sure why doing this properly (even at runtime) should be soo 
> complicated.
>>  When building the getRequestPath() we scan the css if it contains “#{“ and
>>  if so, it cannot be served restfully. In this case we just return the
>>  default request URI from the underlying wrapped ResourceHandler. This
>>  info (static/dynamic) can also easily get cached by using the fully
>>  expanded restful URL as cache key. Thats another 15 lines of code … Why
>>  do we need 10 classes for that?
>> 
> 
> I don't think that's complicated. There is one custom InputStream that 
> checks
> the output for #{ and if it found something, it try to evaluate, if is success
> it output the result, if it is not, it continues.
> 
> What to do? find a bad #{ in a .css looks very, very unlikely. I don't see 
> any
> problem to let this processing as default.
> 
>>  * Prefix mapping automatic detection:
>>  a.) there is no JIRA for it.
>>  b.) This clashes with the REST approach. Read Jakobs original intent and my 
> explanation above. It’s just not needed. If this will some day finally
>>  be included in JSF itself, then we might use the resource serving
>>  mechanism - but then again this will all reside in myfaces-core anyway.
>>  Until then we don’t need it.
>> 
> 
> This point was discussed on dev list and this is the first time I hear these
> arguments. This utility is for 2.0.x and 2.1.x, right?. If the spec includes it
> or not in a later version, we still have the problem. I think your previous
> explanations are a "use case", but this is a library thought to deal 
> with
> multiple use cases. If you don't need is ok, but that's not enough to
> conclude this is not useful.
> 
> The intention is not solve just one use case, is build a extended resource
> handler that can be applied to many different use cases.
> 
> Could you please be more specific about why this clashes with REST
> approach?
> 
>>  * faces-config.xml parsing: not needed imo. What do we need it for? (there 
> goes another 15 classes)
>> 
> 
> Maybe you're saying web.xml parsing. That's for prefix mapping automatic
> detection.
> 
>>  * - Override request path generation using EL expressions
>>  a.) there is no JIRA for it!
>>  b.) What do we need this for? (in hindsight of the explanation above)
>>  I agree that it could be usable to have this configurable - but why via EL? 
> Just too complicated.
>> 
> 
> I don't think so. In faces-config.xml files we use EL for navigation rules.
> Suggestions are welcome.
> 
>>  * - Redirect library names to prevent duplicate usage against other JSF 
> libraries.
>>  a.) there is no JIRA for it!
>>  b.) actually I didn’t get that point.
>> 
> 
> Suppose you use the same javascript library in two different jsf
> libraries, how can
> you make them work together without redirect one library? Otherwise the same
> js file will be included twice on the same page.
> 
>>>  To support this point check this:
>>  You didn’t even gave Jakob a chance to fix that after he imported his code. 
> You created MFCOMMONS-33 at 13/Jun/11 21:11 and checked in all the
>>  ‘changes’ (effectively dropping jakobs logic) at Jun 13 21:12:27 thus
>>  making it ummaintainable in just 1 minute...
>>  You really just overnight ‘fixed’ the code by completely changing it’s 
> meaning and imported tons of classes from other projects.
> 
> But a mail was written on May 18 and before suggesting some changes. For more
> than 1 month I didn't receive any feedback. Then I committed the change but
> I never removed the code. Then I started a discussion about this.
> Everything has
> been done in public. If you don't like something and you don't mention
> it, too bad,
> it is your problem. But anyway it is absurd to discuss about the past. I'm 
> more
> interested about the present and the future.
> 
>>  Especially the
>>  // (FIXME this must be done in a better way when changing
>>  // JSF's own ResourceHandler in JSF 2.2)
>>  was most probably meant that this must get changed IF this logic will be
>>  adopted for the JSF-2.2 specification. This was most probably never
>>  meant that we need to do this for JSF-2.1 and below!
>> 
>>  THAT would have required a VOTE!
>> 
> 
> But it is evidence that the code has design mistakes, and is not using
> the API as it is supposed.
> 
>> 
>>>  which put everything on resourceName without decode it.
>>  Leo really, thats EXACTLY the trick!
>>  The resourceName will just not be the ‘name’ but the fully blown restful 
> indicator!
>>  resourceName of our resourcehandler != resourceName of JSF resource 
> handling!
>> 
> 
> I know, but it is a terrible trick. I have told this before, but that
> strategy leads
> with some ambiguities.
> 
> ".... I think the opposite in this case, because the previous syntax is
> ambiguous, so you can't decide how to get the libraryName and
> resourceName from the resourceBasePath, and the spec requires
> describe that in a explicit way. Think about a resource on:
> 
> /de/mydir/myresource.js (resourceName="de/mydir/myresource.js")
> 
> will produce this request path:
> 
> http://{server}[:port]/{appPath}/javax.faces.resource/de_AT/mydir/myresource.js
> 
> The algorithm will detect de as a locale prefix, mydir as a library and
> myresource.js as a resource name, but that's wrong because the
> resource name is de/mydir/myresource.js. ...."
> 
> Are you willing to continue promoting a hack with so many holes, just because
> makes your code smaller? why don't do things right?, and use resource 
> handler
> API as expected, without any bad shorcuts.
> 
>> 
>>  I suggest to put a VOTE on renaming what we currently have in SVN and
>>  re-import Jakobs original sources and cleanly start the work from there.
>>  Really, we should get back to discuss about what is needed - and once we 
> found
>>  that then we can implement it. Just defining new features and checking
>>  them in 1 minute later just means is not ok. I think this should get
>>  reverted.
>> 
> 
> I think that we should agree first on which features should be
> included. You can
> create a branch for this one and start work there on the mean time.
> But first it is
> necessary to provide reasons why that hack is the way to go. At the end this
> discussion should be about strong and well thought technical reasons. I just
> can't support that vote knowing its foundation is so weak. I'm always +1 
> to
> make things better, even if that means start over again from scratch. But at
> this moment, my reasoning makes me conclude the code we have on
> the svn right now is ok, and if we apply the previous suggestions (remove gzip
> code) the code will look better and easier to understand.
> 
> regards,
> 
> Leonardo Uribe
> 
>>  LieGrue,
>>  strub
>> 
>> 
>> 
>> 
>>  ----- Original Message -----
>>>  From: Leonardo Uribe <lu4242@gmail.com>
>>>  To: MyFaces Development <dev@myfaces.apache.org>; Mark Struberg 
> <struberg@yahoo.de>
>>>  Cc:
>>>  Sent: Friday, October 28, 2011 3:17 AM
>>>  Subject: Re: [DISCUSS] why do we overcomplicate our code soooo much?
>>> 
>>>  Hi
>>> 
>>>  I think you have the wrong impression, basically because you 
> haven't
>>>  tried to solve each one of the problems that the code try to solve. Is
>>>  like the blind that try to see the elephant and say that is like a
>>>  tree, because it touching its leg.
>>> 
>>>  To keep things simple, I'll describe and explain one by one each
>>>  problem "without mercy". Please don't get me wrong.
>>> 
>>>  1. Which features should be included: The original functionality
>>>  proposed by Jakob on the first mail was this:
>>> 
>>>  - relative paths between resources (css files referencing images
>>>  without using #resource['..'])
>>>  - caching resources in the client (disabled if ProjectStage == 
> Development)
>>>  - GZIP compression and local cache in tmp dir (disabled if
>>>  ProjectStage == Development)
>>>  - i18n (supporting country code and language).
>>> 
>>>  The current list is this:
>>> 
>>>  - GZIP compression and local cache in tmp dir.
>>>  - i18n (supporting country code and language).
>>>  - Prevent use #{resource['...']} on css files, using prefix 
> mapping
>>>  - Prefix mapping automatic detection
>>>  - Override request path generation using EL expressions
>>>  - Redirect library names to prevent duplicate usage against other JSF 
> libraries.
>>> 
>>>  If you want to get rid a feature by whatever reason please send a vote
>>>  describing the reasons why that feature is not worth to include it and
>>>  should be removed on the next version.
>>> 
>>>  The first thing we need to do is come to an agreement about how that
>>>  should looks like. As simple as that.
>>> 
>>>  2. You have the impression that the original code provided by Jakob or
>>>  the code in apache extras works correctly. I doubt that. Probably it
>>>  works on simple cases but the code has 'holes' that can't 
> be fixed
>>>  without take the code in myfaces core about resource handler and reuse
>>>  it. To support this point check this:
>>> 
>>>      @Override
>>>      public Resource createResource(String resourceName, String
>>>  libraryName, String contentType)
>>>      {
>>>          FacesContext facesContext = FacesContext.getCurrentInstance();
>>>          String requestedLocalePrefix = null;
>>> 
>>>          // check if libraryName is null and the current request is a
>>>  resource request
>>>          // (FIXME this must be done in a better way when changing
>>>  JSF's own ResourceHandler in JSF 2.2)
>>>          if (libraryName == null &&
>>> 
> facesContext.getApplication().getResourceHandler().isResourceRequest(facesContext))
>>>          {
>>>              // extract the libraryName and the locale from the 
> resourceName
>>>              // --> valid resource url: 
> de/library/Name/resourceName.css
>>> 
>>>              // trim any slashes at begin or end
>>>              resourceName = ResourceUtils.trimSlashes(resourceName);
>>> 
>>>  Can you see what's wrong? Inclusive it has a note that says:
>>> 
>>>          // (FIXME this must be done in a better way when changing
>>>  JSF's own ResourceHandler in JSF 2.2)
>>> 
>>>  The problem is the code try to extract the libraryName from
>>>  resourceName field. Why?, because the original proposal try to change
>>>  the way a resource request path is generated, and make use of a 
> "side
>>>  effect" when that request path is interpreted by the default
>>>  ResourceHandler, which put everything on resourceName without decode
>>>  it.
>>> 
>>>  If you want to do it right, and have control about what's going on 
> it
>>>  is necessary to override ResourceHandler.handleResourceRequest()
>>>  method. That means reuse a lot of code that comes from the default
>>>  resourcehandler implementation and change it so libraryName and
>>>  resourceName can be decoded correctly and createResource() method can
>>>  be called with the right params !!!!!!.
>>> 
>>>  As you can see, all metrics you mention doesn't say anything about
>>>  correctness, and that's the most important thing. This fact, makes 
> all
>>>  previous conclusions you claim pure lies, that does not resist any
>>>  analysis.
>>> 
>>>  3. ".... containing lots of functionality which we NEVER will 
> need!
>>>  ..." who says that? you? do you have any probe about that? Please 
> be
>>>  more specific, otherwise it is not possible to take this kind of
>>>  comments seriously.
>>> 
>>>  I think the best way to solve this is discuss on this thread point by
>>>  point which features should be excluded and the reasons behind that.
>>>  Then, when we have an agreement, I invite you to try to create a
>>>  resource handler that implement such features and then compare it with
>>>  the proposed implementation with the same features. In that way we
>>>  have a fair comparison and finally the community will benefit of such
>>>  effort to make things better.
>>> 
>>>  regards,
>>> 
>>>  Leonardo Uribe
>>> 
>>>  2011/10/27 Mark Struberg <struberg@yahoo.de>:
>>>>     Hi folks!
>>>> 
>>>>   I'm just comparing the original proposal of the resource 
> handler from
>>>  Jakob (which works fine)
>>>> 
>>>> 
>>> 
> http://code.google.com/a/apache-extras.org/p/relative-resource-handler/source/checkout
>>>> 
>>>>   and what we do have now in myfaces-commons-resource-handler
>>>> 
>>>> 
>>> 
> https://svn.apache.org/repos/asf/myfaces/commons/trunk/myfaces-commons-resourcehandler
>>>  (has problems with multiple servlets - fixed that itmt)
>>>> 
>>>> 
>>>>   and to be honest - I'm freaking out a bit!
>>>>   Blowing up the code (with almost the same functionality) to 10 
> times the
>>>  amount of code and copying tons of code from our other projects over 
> cannot be
>>>  the right solution. Really, please be honest and just compare the 
> functionality
>>>> 
>>>>   original config parsing: 110 LOC, working well (java6 specific)
>>>> 
>>>>   new version: 20 classes with ~1800 LOC!
>>>> 
>>>>   And all that just to support java5? I cannot believe that! Even if 
> I need
>>>  to hack all this myself (without any JAX parser or other utility), I 
> don't
>>>  need more than 200 LOC!
>>>> 
>>>>   -> if there is no really good explanation then lets throw the 
> crap away.
>>>> 
>>>> 
>>>>   next stop:
>>>> 
>>>> 
>>>>   package org.apache.myfaces.commons.resourcehandler.resource;
>>>> 
>>>>   original: 3 classes, ~1000 LOC, working
>>>> 
>>>>   myfaces-commons-resource-handler: 13 classes, ~ 2400 LOC
>>>> 
>>>>   containing lots of functionality which we NEVER will need!
>>>> 
>>>>   But at least this code is undocumented (almost no single javadoc) 
> and
>>>  contains no unit tests </sarcasm>
>>>> 
>>>> 
>>>>   I'm really in the mood to rollback this project and start if 
> all over
>>>  again...
>>>> 
>>>> 
>>>>   Really, please, the projects intention was to REPLACE the 
> overloaded crap
>>>  we have in the JFS spec as resource handler with a LIGHT and powerful
>>>  alternative. Thus this shall not get bloated and filled with crap 
> copied all
>>>  over the place.
>>>> 
>>>> 
>>>>   LieGrue,
>>>>   strub
>>>> 
>>>> 
>>> 
>> 
>

Mime
View raw message