Improved: WidgetWorker should not write generated html to Appendable (OFBIZ-11907)

[danwatford]

Refactoring of WidgetWorker so that it generates URI and JSoup Element
objects to represent created URLs, hidden forms and anchor tags. This
replaces the previous approach where WidgetWorker would write string
representations of the URLS, hidden forms and anchor tags directly to an
Appendable object passed to it.

Callers to WidgetWorker have been modified to render the new objects
created by WidgetWorker to their relevant I/O.

[JacquesLeRoux]

Hi Daniel,

Sorry, "This branch has conflicts that must be resolved" as reports GH

[danwatford]

Hi @JacquesLeRoux , conflict has been fixed now.

[JacquesLeRoux]

Hi Daniel,

Sorry again, "This branch has conflicts that must be resolved" as reports GH. I guess it will be easier to fix for you. TIA :)

[danwatford]

All fixed @JacquesLeRoux

[JacquesLeRoux]

Thanks Daniel,

I'll try to have a look today, at least ASAP... before asking people to not change things in this area...

[JacquesLeRoux]

Hi Daniel,

Sorry again, of course I want to review before pushing.

I don't like reviewing in GH (web). I don't like either the GH "command line instructions" because it suggests to merge and then you don't have an easy access to the changes.

I prefer using a patch and do the review in the IDE or such while and after patching and testing.

Unfortunately with https://patch-diff.githubusercontent.com/raw/apache/ofbiz-framework/pull/217.patch I face 2 failing hunks. One "MacroFormRendererTest.java.rej" is not a problem but it shows that your last update misses "import static org.hamcrest.MatcherAssert.assertThat;".

The other "WidgetWorker.java.rej" is problematic because it's a huge one (see attachment). Please update again against trunk HEAD, TIA

[JacquesLeRoux]

OK, GH is not able to show the zipped WidgetWorker.java.rej.zip file in preview above despite saying it's uploaded :/.

Trying here rather... OK same "Error rendering preview" in Preview. Not sure how to see this file, trying a trick with changing extension to txt...

Nothing works or I don't understand how it works :/ Maybe because I use an old FF version. Trying with Edge...

WidgetWorker.java.rej.txt

OK works :)

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

[danwatford]

I'm don't know how to resolve this @JacquesLeRoux .
Before the most recent merge commit a few moments ago I attempted to apply the patch you linked to against trunk and saw the failures you mentioned. As far as I can tell they are due to a difference in whitespace in the trunk compared to the lines expected to be replaced by the patch.

I then tried generated another patch using 'git format-patch trunk --stdout > OFBIZ-11907.patch', but saw the exact same problem with then applying the new patch to trunk.

The issue appears to be with the whitespace is this part of the patch:

-        String tokenValue = CsrfUtil.generateTokenForNonAjax(request, target);
-        if (UtilValidate.isNotEmpty(tokenValue)) {
-            String currentString = externalWriter.toString();
-            if (currentString.startsWith("<form")) {
-                currentString = currentString.substring(currentString.lastIndexOf("\"")+1);
-            }
-            if (currentString.indexOf('?') == -1) {
-                externalWriter.append("?" + CsrfUtil.getTokenNameNonAjax() + "=" + tokenValue);
-            } else {
-                externalWriter.append("&amp;" + CsrfUtil.getTokenNameNonAjax() + "=" + tokenValue);
-            }
+        additionalParameters.forEach(uriBuilder::addParameter);
+
+        try {
+            return uriBuilder.build();
+        } catch (URISyntaxException e) {
+            final String msg = "Syntax error when building URI: " + uriBuilder.toString();
+            Debug.logError(e, msg, MODULE);
+            throw new RuntimeException(msg, e);

If a way to resolve this can be found then I think the patch will apply cleanly.

Perhaps this is a reason to coordinate large formatting changes across the code base as this is the third time I've tried to fix the merge and we are now getting hit by some subtle diff/patch issue that I'm unable to definitively diagnose.

[danwatford]

Hi @JacquesLeRoux , If you haven't been able to work with the patch from this PR, please reject it and I'll re-implement the changes against trunk which should hopefully result in a clean patch.

Thanks,
Dan.

[JacquesLeRoux]

HI Daniel,

That would be perfect, working and updating from trunk HEAD should work, TIA