 |
History
| Log In
|
|
|
|
Ugh, wish I had a patch for ya. If it helps, it works correctly in the latest XWork 1, so perhaps you can go back through the history and identify what/why it was changed...
This is a patch proposal for this and the linked issue.
As you can see, the failing package configurations are stored in a temporary list. The failure list is recursively processed after the initial configuration has been loaded... This should solve the above issue. Jasper, could you please apply the patch and see if this works for you? tia, Rainer Hi Rainer,
I'm afraid that I am out until next Wed. There seem to be lots of interested parties to this issue though (given all the related issues in struts 2), so you might want to see if you can get someone else to check out the patch if you want an quicker review. It does sound like a clever solution though and, based on your description, will have the nice side-effect of having package order no longer matter at all (whereas in XWork 1 the package order did have to be sequential if you imagined flattening all the imported files.) -Jasper Hi Rainer,
The patch looks like it will work, but I do have some suggestions for improvement (this is by inspection rather than execution, so hopefully I didn't misread the code): 1. In XmlConfigurationProvider.loadPackages(), the reloads.clear() call is unnecessary. 2. In XmlConfigurationProvider.reloadRequiredPackages(), I think that you want to return directly after the recursive call, otherwise you will get logging errors for packages that were actually resolved in the recursive invocation. 3. If I understand the current flow correctly, it will end up reprocessing all packages in a document that might only have a subset of packages that had parents that were not yet declared. In fact, because a doc gets processed for every package where we can't find a parent, the same package could end up processed many times over. For example, a single document with packages declared in the following order: package C extends B package B extends A package A i. C, B, and A are processed, C and B are to be reloaded, A is successfully added. ii. C's doc is processed, so C, B, and A are all processed again, C is added to "result" to be reloaded recursively, B is successfully added, A is overwritten. iii. B's doc is processed, so C, B, and A are all processed again, C is successfully added, A and B are overwritten. iv. C's doc is processed again (from recursive call), so C, B, and A are all processed again and A, B, and C are overwritten. Ideally, this could be avoided by reloading the children packages directly rather than reprocessing their entire parent document for each package whose parent was not found. The one complication to this is the loadExtraConfiguration(doc) call which I'm not sure belongs conceptually in reloadRequiredPackages() anyway. Should it be allowed to be called multiple times per document? Also, does it need to be called in document load order? Perhaps it should be factored out to its own loop to be called a single time on each document after the call to reloadRequiredPackages() in loadPackages(). Hope something in here is helpful! -Jasper Jasper,
thanks for your comments. 1) true, just helping the garbage collector here :) 2) This one is already resolved. The patch has been improved and already commited. So the logging should only apply, when the package could not be loaded at all. 3) For clarification, an already processed package will not be processed again. Therefore I added the "needsRefresh" boolean property to PackageConfig. Only "dirty" packages will be checked and if possible initialized. Only the document is reparsed several times... Regarding the loadExtraConfiguration, this sounds reasonable to me, but I won't have time to look into this before next weekend. If you could provide a patch with your suggestion I'd be happy to apply it. tia, Rainer Hi Rainer,
Here is a patch that includes all of my suggested changes. It is against the trunk. Processing at the package level rather than the doc level greatly simplified things... Thanks, Jasper Oh also, I did rerun your new unit test against my patch.
Regarding your responses: 1) true, just helping the garbage collector here :) I suppose technically the garbage collector could run in the half a ms before it goes out of scope on its own ;) 2) This one is already resolved. The patch has been improved and already commited. So the logging should only apply, when the package could not be loaded at all. I'm not seeing this in svn, perhaps there is some lag, but in any case it is handled by my patch as well. 3) For clarification, an already processed package will not be processed again. Therefore I added the "needsRefresh" boolean property to PackageConfig. Only "dirty" packages will be checked and if possible initialized. Only the document is reparsed several times... I guess I would need to step through the code to be sure, but looking at addPackage(), I'm not seeing anywhere where it recognizes that a package was previously parsed. It just looks to me like it will be reprocessed and then overwrite the earlier one... I guess I must be missing something... | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Resolved
Major
Packages in includes can't see packages declared ahead of include
I have a unit test showing the problem, but it will take a couple of hours, since my time is limited...
The whole configuration loading needs to be reworked, if I got it right.
You do not have a patch arround to solve this issue? :)
cheers,
Rainer