[FIXED JENKINS-36315] Make it easier to infer the context in form binding #62

stephenc · 2016-07-04T15:27:01Z

@reviewbybees

…ding

stephenc · 2016-07-04T15:28:07Z

src/main/java/com/cloudbees/plugins/credentials/CredentialsDescriptor.java

+                                return type.cast(user);
+                            }
+                        } else if ("job".equals(firstSegment) || "item".equals(firstSegment) || "view"
+                                .equals(firstSegment)) {


@jglick you may be interested in this section as a better implementation of https://github.com/jenkinsci/workflow-step-api-plugin/blob/aabbdca1c3c1f1aacf346b01d95d3be467547195/src/main/java/org/jenkinsci/plugins/workflow/util/StaplerReferer.java#L61

:sadpanda: I've had need in the past fo something similar in Stapler as well
https://github.com/jenkinsci/build-failure-analyzer-plugin/blob/master/src/main/java/com/sonyericsson/jenkins/plugins/bfa/model/indication/BuildLogIndication.java#L263

yeah... mine is better I think ;-)

Yes, I didn't write mine myself and IIRC it doesn't work well with views and folders :) But hardly none has complained, probably because it isn't a well known feature.

ghost · 2016-07-04T15:31:43Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

oleg-nenashev · 2016-07-04T15:44:49Z

src/main/java/com/cloudbees/plugins/credentials/CredentialsDescriptor.java

-            if (ancestor.getObject() instanceof ModelObject) {
-                Set<CredentialsScope> scopes =
-                        CredentialsProvider.lookupScopes((ModelObject) ancestor.getObject());
+                Set<CredentialsScope> scopes = CredentialsProvider.lookupScopes(context);


NIT. formatting. Or maybe dead code?

oleg-nenashev · 2016-07-04T15:57:01Z

src/main/java/com/cloudbees/plugins/credentials/CredentialsDescriptor.java

+     * {@inheritDoc}
+     */
+    public FormValidation.CheckMethod getCheckMethod(String fieldName) {
+        // this is an ugly hack to make the @ContextInPath annotation more failsafe


Should this hack be restricted (from direct override calls)?

oleg-nenashev · 2016-07-04T16:01:00Z

src/main/java/com/cloudbees/plugins/credentials/CredentialsDescriptor.java

+                        if (token != null) {
+                            String fillUrl = (String) attributes.get("autoCompleteUrl");
+                            if (fillUrl.indexOf('?') != -1) {
+                                fillUrl = fillUrl + '&';


Check for null

(just because the three known impls are safe does not mean that some plugin won't introduce an extension that needs escaping)

rsandell · 2016-07-04T16:12:39Z

src/main/java/com/cloudbees/plugins/credentials/CredentialsDescriptor.java

+        // this is an ugly hack to make the @ContextInPath annotation more failsafe
+        super.calcFillSettings(field, attributes);
+        if (attributes.containsKey("fillUrl")) {
+            try {


Same check as above, so this code will not run.

nope... the calcFillSettings method will add the attribute.

The idea is to do a no-op if the user supplied the fillUrl, but if the fillUrl was calculated then we add the $provider and $token

Then 🐜 A comment mentioning that would be nice ;)

you mean like // the user already provided a custom one, get out of the way

oleg-nenashev · 2016-07-04T16:18:13Z

src/main/java/com/cloudbees/plugins/credentials/impl/BaseStandardCredentials.java

                        } else {
+                            if (match.getScope() == CredentialsScope.SYSTEM) {
+                                // system scope is not exported to child contexts
+                                continue;


🐛 Should it continue with other credentials in the domain then? firstOrNull skips others from what I see

rsandell · 2016-07-04T16:23:09Z

src/main/java/com/cloudbees/plugins/credentials/CredentialsDescriptor.java

+        // this is an ugly hack to make the @ContextInPath annotation more failsafe
+        super.calcAutoCompleteSettings(field, attributes);
+        if (attributes.containsKey("autoCompleteUrl")) {
+            try {


So, seems like only a one string difference between this and the code for fillUrl above. Reducing the "hack" to a one method call would help with scrubbing it in the future.

rsandell · 2016-07-04T16:28:21Z

🐝

oleg-nenashev · 2016-07-04T16:30:21Z

I keep was-bug (fixed) for checkForDuplicates() domain handling and className escaping, but the most of the issues have been addressed

…is non-visible

… ids

oleg-nenashev · 2016-07-04T17:29:49Z

"Let's all share core's lack of escaping of descriptor ids" looks good to me since there is no flaws in the known OSS code

stephenc · 2016-07-04T18:25:46Z

@oleg-nenashev are you removing your bug then?

oleg-nenashev · 2016-07-04T18:33:14Z

Removed bug, need to perform another iteration before adding a bee

rsandell · 2016-07-05T10:31:12Z

🐝

jglick · 2016-07-07T20:08:44Z

...oudbees/plugins/credentials/CredentialsSelectHelper/WrappedCredentialsStore/credential.jelly

-    </j:forEach>
-  </f:dropdownList>
+  <j:local>
+    <j:set var="it" value="${it.store}"/>


Do you really need j:local here? AFAIK it would have been scoped only to this include anyway.

I'm curious about this j:local -- it bleeds into the html that web browsers see. That isn't inherently bad, but in general one shouldn't bleed xml namespaced tags to html web browser.

It shouldn’t, assuming the xml namespace is correctly defined

I don't see a <local> tag in the Jelly documentation

Ahh should be scope http://commons.apache.org/proper/commons-jelly/tags.html#core:scope Do you want to fix it?

jglick · 2016-07-07T20:14:06Z

The JENKINS-19413 workaround does look like a modest improvement over the existing implementations (there are at least two copies lying around that I know of); these should I think be moved into a library plugin for eventual inclusion in core (or Stapler itself). Obviously the better approach would be to directly reuse the URL-binding logic in Stapler.tryInvoke, which would seem to require an implementation in Stapler core itself.

The rest of the PR I do not really understand. Something about mismatched its. Do not see anything obviously wrong, other than the usual nightmare of making form validation work in exotic circumstances.

[FIXED JENKINS-36315] Make it easier to infer the context in form bin…

5d985eb

…ding

stephenc reviewed Jul 4, 2016
View reviewed changes

oleg-nenashev reviewed Jul 4, 2016
View reviewed changes

stephenc added 2 commits July 4, 2016 16:53

[JENKINS-36315] Formatting

b0444dd

[JENKINS-36315] Nullness

e16af99

oleg-nenashev reviewed Jul 4, 2016
View reviewed changes

[JENKINS-36315] Noting core issue (JENKINS-19413)

b616485

oleg-nenashev reviewed Jul 4, 2016
View reviewed changes

[JENKINS-36315] Encode everything

660aeef

(just because the three known impls are safe does not mean that some plugin won't introduce an extension that needs escaping)

rsandell reviewed Jul 4, 2016
View reviewed changes

[JENKINS-36315] Bail fast

a021baf

oleg-nenashev reviewed Jul 4, 2016
View reviewed changes

rsandell reviewed Jul 4, 2016
View reviewed changes

stephenc added 2 commits July 4, 2016 17:31

[JENKINS-36315] Continue checking for other visible matches if first …

7ccf111

…is non-visible

[JENKINS-36315] Let's all share core's lack of escaping of descriptor…

e4e6a08

… ids

[JENKINS-36315] Remove dead code

cdcc1c5

jglick reviewed Jul 7, 2016
View reviewed changes

stephenc merged commit b0780d1 into master Jul 11, 2016

stephenc deleted the jenkins-36315 branch January 26, 2017 16:42

jsoref mentioned this pull request Feb 5, 2019

replace j:local with j:scope #109

Merged

jglick mentioned this pull request Sep 11, 2025

[JENKINS-19413] Use CredentialsDescriptor.findContextInPath rather than directly StaplerRequest2.findAncestorObject #651

Merged

[FIXED JENKINS-36315] Make it easier to infer the context in form binding #62

[FIXED JENKINS-36315] Make it easier to infer the context in form binding #62

Uh oh!

Conversation

stephenc commented Jul 4, 2016 • edited by jglick Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsandell Jul 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsandell commented Jul 4, 2016

Uh oh!

oleg-nenashev commented Jul 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oleg-nenashev commented Jul 4, 2016

Uh oh!

stephenc commented Jul 4, 2016

Uh oh!

oleg-nenashev commented Jul 4, 2016

Uh oh!

rsandell commented Jul 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slide Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick commented Jul 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

stephenc commented Jul 4, 2016 •

edited by jglick

Loading

rsandell Jul 4, 2016 •

edited

Loading

oleg-nenashev commented Jul 4, 2016 •

edited

Loading

slide Feb 4, 2019 •

edited

Loading