Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for shared lib and params variables being null in environment section #529

Merged
merged 1 commit into from
Jul 7, 2022

Conversation

tomek-d
Copy link
Contributor

@tomek-d tomek-d commented May 29, 2022

  • 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

Hello, this is a possible fix for #387.

Analysis of the issue

According to Groovy's metaprogramming rules (https://docs.groovy-lang.org/latest/html/documentation/core-metaprogramming.html) Groovy calls getProperty method on Closure which in turn tries to get this property from the delegate (in our case this is a Map) and when the property is not found it tries to find the property on the owner object.
Here's code snippet from Groovy's source code:

    private Object getPropertyTryThese(String property, Object firstTry, Object secondTry) {
        try {
            // let's try getting the property on the first object
            return InvokerHelper.getProperty(firstTry, property);

        } catch (MissingPropertyException | MissingFieldException e1) {
            if (secondTry != null && firstTry != this && firstTry != secondTry) {
                try {
                    // let's try getting the property on the second object
                    return InvokerHelper.getProperty(secondTry, property);
                } catch (GroovyRuntimeException e2) {
                    // ignore, we'll throw e1
                }
            }
            throw e1;

        }
    }

https://github.com/apache/groovy/blob/02d1a3b4f76f1311eb2286188b224cfbbf0324db/src/main/java/groovy/lang/Closure.java#L322

In our case delegate is a Map and property retrieval for a Map is implemented as a call to get method on that Map as shown on following code snippet from Groovy implementation:

if (isMap && !isStatic) {
	return ((Map<?,?>) object).get(name);
}	

https://github.com/apache/groovy/blob/master/src/main/java/groovy/lang/MetaClassImpl.java#L2001

The problem here is that Map returns null when the property is not found however Closure expects MissingPropertyException or MissingFieldException exceptions to try to get the property from the owner.

Proposed fix

Create a wrapper that first checks if the property is defined in a map and if not then it tries to get property from the owner which in this case is an instance of GenericPipelineDeclaration.

PS. I'm not a Groovy developer so possibly a better solution could be found.

@nre-ableton
Copy link
Contributor

Sorry, for the delay, I haven't had time to work on this project recently, but my team now has a hack sprint so I'll be examining this issue soon.

Copy link
Contributor

@nre-ableton nre-ableton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not particularly familiar with this part of the framework, but thanks for the thorough explanation and providing the test cases. Everything here seems pretty reasonable to me -- I hope I'm not overlooking something. 😅

@nre-ableton nre-ableton merged commit 9ecdf79 into jenkinsci:master Jul 7, 2022
@tomek-d
Copy link
Contributor Author

tomek-d commented Jul 11, 2022

@nre-ableton - thanks for merging this PR ;)

@tomek-d tomek-d deleted the bugfix/null-env-var branch July 11, 2022 19:13
@nre-ableton
Copy link
Contributor

@tomek-d No problem! I'll probably make a release sometime at the end of this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants