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). 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 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... We imo don’t need (points from your list): * 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. * 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! * Prevent use #{resource['...']} on css files, using prefix mapping a.) there is no JIRA for it. 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? * 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. * faces-config.xml parsing: not needed imo. What do we need it for? (there goes another 15 classes) * - 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. * - 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. > 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. 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! > 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 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. 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 >> >> >
