Manifest access cleanup

[rgoldberg]

Same as old PR #349 but without enabling enableDigests or skipNonDigests

[rgoldberg]

CI failure is probably because of 2.5.4 wrapper issue also mentioned in #351

[davydotcom]

It was intentional that the servlet filter be entirely self sufficient and not utilize any grails specific beans. This is the one area i would not touch with regards to accessing the assetProcessorService

[davydotcom]

Ok so this PR looks great save for one thing. It was intentional that the servlet filter not access an assetProcessorService bean. It was intended that the servlet filter become entirely independent of any grails specific services or beans. As a matter of fact the grails3 version will be moving to the asset-pipeline-servlet to further improve performance and reduce duplication. As far as converting things to final and static typing this is great as it provides the potential for moving modules to @CompileStatic.

[rgoldberg]

If I move getAssetPath(...) from AssetProcessorService to some other POJO or POGO class as a static method, will that work? If so, what class should I move it into?

Or should I just make it a static method in AssetProcessorService?

[davydotcom]

all it does is grab from the manifest whats the point in adding call stack execution for this?

[rgoldberg]

1 Code reuse prevents bugs. The existing copied code had bugs (or at least unnecessary code). e.g., if manifest is null, then fileUri is incorrectly set to null by:

fileUri = manifest?.getProperty(manifestPath, manifestPath)

If manifest can be null, it should be:

fileUri = manifest?.getProperty(manifestPath) :? manifestPath

If it cannot be null, it should be:

fileUri = manifest.getProperty(manifestPath, manifestPath)

2 A single line call is easier to read than 5 lines of code

3 If you ever make a change to how you get an asset path, code reuse allows you to make that change in one place and automatically apply it everywhere. A while ago, I fixed a problem where initial slashes weren't removed from asset URIs. Code reuse allows that change to be made once in one place. Besides having to fix copied code in many places, copied code can get out of sync.

4 For normal Java (I don't know about Groovy), method calls are inlined if there are no overriding methods. So a call stack execution isn't added. And even if it were, a call stack execution (at least in Java) is very quick. There are many worse performance inefficiencies than a single method call that can be fixed up more easily by having cleaner code.

[davydotcom]

1. In that context manifest.properties can NOT be null (production always has a manifest).
2. As this code is being rolled into the generic servlet for grails 3 this causes a divergence more than a benefit in saving 5 lines of code.
3. I will make it in one servlet interface in the long rung.
4. Groovy does typically end up wrapping methods especially possible in service beans and was intentionally kept out.