refactor: Replace synchronous proxy with standard proxy #79

rahulsom · 2023-07-04T20:02:38Z

The code in makeStaplerProxy2 used prototype.js to create a synchronous version of the standard proxy that comes with Jenkins Core.

This code switches back to the standard proxy.
That requires making the code execute in the correct order when there were dependencies. That is achieved by using async/await in the .es6 code and transpiling it down to use Promises so HtmlUnit tests continue to work.

Refs: JENKINS-71365

Testing done

The existing tests pass. The UI Acceptance test was relying on an unexpected side-effect of synchronous network calls.
To account for the network calls becoming asynchronous, the tests now wait for the condition to be successful instead of a straight assert.

Submitter checklist

Give feedback

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue
Options

src/test/java/org/biouno/unochoice/UiAcceptanceTest.java

basil

Very nice! Highly appreciate this contribution.

The test fails on 2.413 when run with mvn clean verify -Dtest=org.biouno.unochoice.UiAcceptanceTest -Djenkins.version=2.413. The same test passes on 2.413 on the main branch. Console logs:

UnoChoice.js:1 Values returned from server: undefined
build:1 Uncaught (in promise) SyntaxError: "undefined" is not valid JSON
    at JSON.parse (<anonymous>)
    at UnoChoice.js:1:8886
    at bind.js:56:33

rahulsom · 2023-07-06T23:10:43Z

It looks like we don't receive a responseText anymore. We receive a responseJSON from the stapler proxy.
I can check which we have and either parse or use the parsed object. Does that seem reasonable?

basil · 2023-07-06T23:18:49Z

If the server is returning JSON (which was the original use case supported by Stapler), then the client is expected to call r.responseObject() to get back a JSON object. The r.responseJSON field is only provided for backward compatibility for consumers that were incorrectly assuming the Prototype implementation. Similarly, the r.responseText field is only provided when non-JSON is being returned for backward compatibility for consumers that were incorrectly abusing the Prototype implementation to work with non-JSON values (not originally intended to be supported by Stapler, but supported in practice by the Prototype implementation). So I would recommend calling r.responseObject() and not attempting to parse the JSON.

rahulsom · 2023-07-06T23:39:26Z

Thanks! That is cleaner.

basil

Looks good! I tested the result both with and without Prototype and it seems to be working without Prototype. Just two minor comments before approving this.

pom.xml

src/main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6

basil

Very nice!

pom.xml

mawinter69

The plugin is still using inline javascript. That should loading be changed to conform to CSP.
But this is probably something for a separate PR.

src/main/resources/org/biouno/unochoice/CascadeChoiceParameter/index.jelly

src/main/resources/org/biouno/unochoice/DynamicReferenceParameter/index.jelly

kinow

Didn't have time to check out the code and play with it tonight, but it's looking good! Left a few questions, mostly me learning what's new in Jenkins API & dev :) Thanks!

src/main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6

kinow · 2023-07-07T22:32:40Z

src/main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6

+    }
+    function decrementLoadingCounter() {
+        loadingCounter--;
+    }


I thought it was going to be used to check for consistency, or maybe for the tests, but I couldn't locate where the loadingCounter value is being read. Is that somewhere in this file? Maybe GitHub UI is not showing it in the diff (didn't have time to check it out tonight, sorry).

I did try it. I got rid of it. I'll remove this code. Thanks for the extra pair of eyes on this.

kinow · 2023-07-07T22:35:35Z

src/main/resources/org/biouno/unochoice/CascadeChoiceParameter/index.jelly

@@ -2,6 +2,7 @@
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler">
  ${it.parameters.clear()}
  <st:include page="/org/biouno/unochoice/common/choiceParameterCommon.jelly"/>
+  <st:bind value="${it}" var="cascadeChoiceParameter"/>


👍 I think I used the other ugly syntax to fix warnings on my IDE (Eclipse + WebDev at that time, I think?). Do VSCode/WebStorm understand this? Maybe there's a stapler plug-in that I can install?

I'm on IntelliJ and it seems to understand it.
Also, now most of the JS is out of this file, so there isn't much context left.
I do have this plugin - https://github.com/jenkinsci/idea-stapler-plugin

#79 (comment)

That's the reason I changed this.

Right, inline syntax is not going to work with Content-Security-Policy prohibiting inline scripts. See JENKINS-60865 for context.

kinow

These changes look all good to me, @rahulsom !

I don't have an installation with jobs to test it, but I know @imoutsatsos maintains probably the largest and most complex series of jobs with active choices parameters 🙂

@imoutsatsos would you be able to test this change? If everything works for you we can merge it. No need to look at the code, just try the existing jobs you have, checking if you get any JavaScript errors, or parameters that were supposed to be rendered correctly not being displayed, displaying wrong values, etc.

Thanks!

imoutsatsos · 2023-07-10T22:10:24Z

I will do my best to test the updated AC plugin @bruno P. ***@***.***> but it could take me several days given that I will have to set a test harness separate from my current environments. Is there a minimum Jenkins LTS version required? Sent from my T-Mobile 5G Device Get Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Bruno P. Kinoshita ***@***.***> Sent: Monday, July 10, 2023 1:12:55 PM To: jenkinsci/active-choices-plugin ***@***.***> Cc: Ioannis K. Moutsatsos ***@***.***>; Mention ***@***.***> Subject: Re: [jenkinsci/active-choices-plugin] refactor: Replace synchronous proxy with standard proxy (PR #79) @kinow commented on this pull request. These changes look all good to me, @rahulsom<https://github.com/rahulsom> ! I don't have an installation with jobs to test it, but I know @imoutsatsos<https://github.com/imoutsatsos> maintains probably the largest and most complex series of jobs with active choices parameters 🙂 @imoutsatsos<https://github.com/imoutsatsos> would you be able to test this change? If everything works for you we can merge it. No need to look at the code, just try the existing jobs you have, checking if you get any JavaScript errors, or parameters that were supposed to be rendered correctly not being displayed, displaying wrong values, etc. Thanks! — Reply to this email directly, view it on GitHub<#79 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AA2ACYWAN4IRKLVYUQRQ5ALXPQZZPANCNFSM6AAAAAAZ6DEUEQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

rahulsom · 2023-07-11T01:55:27Z

For this version of the plugin to run, you need 2.375.3 or newer.

There isn't an LTS with the Prototype removal changes yet. You can use 2.413 to verify the change with prototype turned off (a user flag under the /user/{USERNAME}/configure URI).

imoutsatsos · 2023-07-17T19:05:17Z

CONFIRMED: Tests done with AC PR #80. I have completed some functional testing (using 6 different jobs that use Active Choice parameters in different ways) of the 2.6.6 SNAPSHOT with Jenkins 2.375.3 and Windows 2016 Server. Testing results look good. No issues @rahulsom and @kinow . Thank you!

kinow · 2023-07-17T21:01:22Z

Thank you @imoutsatsos ! Just to confirm, the snapshot version you mentioned was built from this branch/pull request? If so we might be ready for a release! Thanks!

kinow · 2023-07-17T21:01:42Z

@rahulsom could you base, please? Thanks 🙇

rahulsom · 2023-07-18T02:29:13Z

@kinow Rebased. We're ready for merging. Thanks!

kinow · 2023-07-18T06:30:07Z

Thanks @rahulsom . Just waiting for Ioannis confirmation that the tests used this branch and then we are ready to merge.

imoutsatsos · 2023-07-22T18:12:28Z

Hi @kinow . Confirming that successful functional test of AC in various jobs were successfully performed with the PR #80 version

The code in `makeStaplerProxy2` used `prototype.js` to create a synchronous version of the standard proxy that comes with Jenkins Core. This code switches back to the standard proxy. That requires making the code execute in the correct order when there were dependencies. That is achieved by using `async`/`await` in the `.es6` code and transpiling it down to use Promises so HtmlUnit tests continue to work. Refs: JENKINS-71365 Co-authored-by: Basil Crow <[email protected]>

kinow

Thank you for testing it, @imoutsatsos ! And thanks for the great work (again!) @rahulsom ! Merging.

kinow · 2023-07-22T19:40:50Z

@rahulsom , I think now we have everything merged, so I am planning to cut a release tomorrow with everything on master. I'll bump to 2.7 instead of 2.6.6 due to some bigger changes in the code (even though they should be internal and transparent to users). You should be able to try that version By Sun avro or Monday morning 👋 Thanks!

kinow · 2024-07-06T09:07:51Z

This is being reverted due to issues caused when parameters are handled out of order.

The user scripts may use document.getElementById, for instance, to access the elements of other parameters, which fails with this change as parameters may be rendered in any order with a proxy.

In order for this to work, there would have to be a JS layer option for users to attach their callbacks/code executed once all parameters have been loaded, or after a certain parameter has been loaded (which is possible in Vue, React, Svelte, etc., but not in pure Jenkins UI + JS).

The merge request mentions JENKINS-71365 (prototype.JS is removed from Jenkins core) but reverting this should not impact that. What we are doing is removing the async calls and promises, and instead using the synchronous code again. This is so that Active Choices parameters are rendering in order as they were before. Whilst rendering them asynchronously seemed like a good idea to provide a more responsive UI, in reality we oversaw an important aspect: users write their scripts relying on the order parameters are loaded. Users wrote their code using document.getElementId, for instance, which worked previously as users would put the parameter doing that after the parameter from which the ID was being used. There were several issues rendered, so this is not likely to be added back again any time soon. In order to have a more smart reactivity, we would need places to hook user callback code (like Vue or React do), but provided by Jenkins UI, or in a way that could take some time to create in the plug-in without breaking things, and that works well.

rahulsom marked this pull request as draft July 4, 2023 20:02

kinow reviewed Jul 4, 2023

View reviewed changes

src/test/java/org/biouno/unochoice/UiAcceptanceTest.java Outdated Show resolved Hide resolved

rahulsom force-pushed the remove-prototype-dependency branch 16 times, most recently from f55a202 to 738a194 Compare July 5, 2023 19:54

rahulsom marked this pull request as ready for review July 5, 2023 20:02

basil reviewed Jul 6, 2023

View reviewed changes

rahulsom force-pushed the remove-prototype-dependency branch from 738a194 to 456a844 Compare July 6, 2023 23:38

basil reviewed Jul 6, 2023

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

src/main/resources/org/biouno/unochoice/stapler/unochoice/UnoChoice.es6 Outdated Show resolved Hide resolved

rahulsom force-pushed the remove-prototype-dependency branch from 604ebc4 to 1e234e0 Compare July 7, 2023 01:28

basil approved these changes Jul 7, 2023

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

mawinter69 reviewed Jul 7, 2023

View reviewed changes

src/main/resources/org/biouno/unochoice/CascadeChoiceParameter/index.jelly Outdated Show resolved Hide resolved

src/main/resources/org/biouno/unochoice/DynamicReferenceParameter/index.jelly Outdated Show resolved Hide resolved

rahulsom force-pushed the remove-prototype-dependency branch from 1e234e0 to ed3398b Compare July 7, 2023 17:39

basil approved these changes Jul 7, 2023

View reviewed changes

kinow reviewed Jul 7, 2023

View reviewed changes

rahulsom force-pushed the remove-prototype-dependency branch 2 times, most recently from 964274e to 44d8b2e Compare July 8, 2023 01:42

kinow reviewed Jul 10, 2023

View reviewed changes

rahulsom force-pushed the remove-prototype-dependency branch from 44d8b2e to 5815d29 Compare July 18, 2023 02:16

rahulsom force-pushed the remove-prototype-dependency branch from 5815d29 to 8fc61b5 Compare July 22, 2023 19:14

kinow approved these changes Jul 22, 2023

View reviewed changes

kinow merged commit fdea6b4 into jenkinsci:master Jul 22, 2023
13 checks passed

kinow mentioned this pull request Jul 31, 2023

Move (most) javascript out of jelly files #76

Merged

asc3ns10n mentioned this pull request Aug 14, 2024

[JENKINS-71909]: Revert https://github.com/jenkinsci/active-choices-p… #298

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Replace synchronous proxy with standard proxy #79

refactor: Replace synchronous proxy with standard proxy #79

rahulsom commented Jul 4, 2023 •

edited

Loading

Submitter checklist

basil left a comment •

edited

Loading

rahulsom commented Jul 6, 2023

basil commented Jul 6, 2023

rahulsom commented Jul 6, 2023

basil left a comment

basil left a comment

mawinter69 left a comment

kinow left a comment

kinow Jul 7, 2023

rahulsom Jul 8, 2023

kinow Jul 7, 2023

rahulsom Jul 8, 2023

rahulsom Jul 8, 2023

daniel-beck Jul 11, 2023

kinow left a comment

imoutsatsos commented Jul 10, 2023 via email

rahulsom commented Jul 11, 2023

imoutsatsos commented Jul 17, 2023 •

edited

Loading

kinow commented Jul 17, 2023

kinow commented Jul 17, 2023

rahulsom commented Jul 18, 2023

kinow commented Jul 18, 2023

imoutsatsos commented Jul 22, 2023

kinow left a comment

kinow commented Jul 22, 2023

kinow commented Jul 6, 2024

refactor: Replace synchronous proxy with standard proxy #79

refactor: Replace synchronous proxy with standard proxy #79

Conversation

rahulsom commented Jul 4, 2023 • edited Loading

Testing done

Submitter checklist

basil left a comment • edited Loading

Choose a reason for hiding this comment

rahulsom commented Jul 6, 2023

basil commented Jul 6, 2023

rahulsom commented Jul 6, 2023

basil left a comment

Choose a reason for hiding this comment

basil left a comment

Choose a reason for hiding this comment

mawinter69 left a comment

Choose a reason for hiding this comment

kinow left a comment

Choose a reason for hiding this comment

kinow Jul 7, 2023

Choose a reason for hiding this comment

rahulsom Jul 8, 2023

Choose a reason for hiding this comment

kinow Jul 7, 2023

Choose a reason for hiding this comment

rahulsom Jul 8, 2023

Choose a reason for hiding this comment

rahulsom Jul 8, 2023

Choose a reason for hiding this comment

daniel-beck Jul 11, 2023

Choose a reason for hiding this comment

kinow left a comment

Choose a reason for hiding this comment

imoutsatsos commented Jul 10, 2023 via email

rahulsom commented Jul 11, 2023

imoutsatsos commented Jul 17, 2023 • edited Loading

kinow commented Jul 17, 2023

kinow commented Jul 17, 2023

rahulsom commented Jul 18, 2023

kinow commented Jul 18, 2023

imoutsatsos commented Jul 22, 2023

kinow left a comment

Choose a reason for hiding this comment

kinow commented Jul 22, 2023

kinow commented Jul 6, 2024

rahulsom commented Jul 4, 2023 •

edited

Loading

basil left a comment •

edited

Loading

imoutsatsos commented Jul 17, 2023 •

edited

Loading