Hi Mark 2011/10/28 Mark Struberg <[email protected]>: > 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 <[email protected]> >> To: MyFaces Development <[email protected]>; Mark Struberg >> <[email protected]> >> 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 <[email protected]>: >>> 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 >>> >>> >> >
