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

Groovy client doesn't bind to any variables outside the closure #654

Closed
spn opened this issue Jan 28, 2011 · 10 comments
Closed

Groovy client doesn't bind to any variables outside the closure #654

spn opened this issue Jan 28, 2011 · 10 comments

Comments

@spn
Copy link

spn commented Jan 28, 2011

I have serious troubles with Groovy client not being able to search for a simple query.
The code is like this:
myVariable = 'search string'
gclient.search {
source {
query {
queryString myVariable
}
}
}

This will fail because myVariable appears to be always null. The reason is that GXContentBuilder violates Groovy property convention in GXContentBuilder.getProperty:
def getProperty(String propName) {
current[propName]
}
This is wrong. If builder does not know the property it is supposed to throw MissingPropertyException. Returning null indicates that property, in fact, exists, but has null value.

@kimchy
Copy link
Member

kimchy commented Jan 28, 2011

I copied the logic from grails JsonBuilder: https://github.com/grails/grails-core/blob/master/src/java/grails/util/JSonBuilder.java. Not a groovy expert, but wanted to get the same "json feeling" as in grails.

@spn
Copy link
Author

spn commented Jan 29, 2011

for me the following worked fine:

  1. Making GXContentBuilder extends GroovyObjectSupport.
  2. Changing getProperty() method to:
    def getProperty(String propName) {
    if (current?.containsKey(propName)) {
    return current[propName]
    } else {
    return super.getProperty(propName)
    }
    }
    https://github.com/spn/elasticsearch-grails-plugin/blob/master/src/groovy/org/grails/plugins/elasticsearch/util/GXContentBuilder.groovy

@kimchy
Copy link
Member

kimchy commented Jan 30, 2011

Interesting!. I can't really judge the implications of this change because of my lack of groovy knowledge :). Wondering why grails are not doing the same? Maybe it makes sense to ping the grails mailing list?

@spn
Copy link
Author

spn commented Jan 31, 2011

well, Grails IS doing the same sometimes eg look at https://github.com/grails/grails-core/blob/master/src/java/grails/spring/BeanBuilder.java

(actually, in your case the class is written in Groovy so you don't need to extend GroovyObjectSupport. But it still has to fallback to standard property resolution)

@kimchy
Copy link
Member

kimchy commented Jan 31, 2011

What I mean is that it does not do it for JsnoBuilder, wondering what the reason for that is, and thought it might make sense to ping the list?

@spn
Copy link
Author

spn commented Feb 1, 2011

That's right. JsonBuilder uses Builder API, which is sort of high-level tree-oriented; it does not require you to manage properties and methods (BuilderSupport class is doing it internally)

@kimchy
Copy link
Member

kimchy commented Feb 1, 2011

I'm sorry, I sent the wrong link to the JsonBuilder used in grails. This is the one ES is based on: https://github.com/grails/grails-core/blob/master/src/java/grails/web/JSONBuilder.groovy .

@spn
Copy link
Author

spn commented Feb 1, 2011

it looks to me you have a small difference from the original JSONBuilder:
private buildRoot(Closure c) {
c.delegate = this
//c.resolveStrategy = Closure.DELEGATE_FIRST
root = [:]
current = root
def returnValue = c.call()
if (!root) {
return returnValue
}
return root
}

Looking at GXContentBuilder, c.resolveStrategy is NOT commented out. This is a serious difference, because it affects delegation rules (by default, closure delegates to OWNER_FIRST, and owner is "client" code which uses variables).

@kimchy
Copy link
Member

kimchy commented Feb 1, 2011

Right, but then it makes a big difference when used in scripts... . Thats why in the builder you can control what it is (static var...). But, I will change it so the default will be OWNER_FIRST.

@kimchy
Copy link
Member

kimchy commented Feb 1, 2011

Groovy client doesn't bind to any variables outside the closure, closed by 0dfa3dc.

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

No branches or pull requests

2 participants