Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

named queries patch #317

Closed
wants to merge 6 commits into from

3 participants

@tudor-malene
  • fixed GRAILS-8963 ( the additional criteria closure was added to the params array )
  • implemented GRAILS-9804
  • added better error reporting when calling named query with wrong no of named criteria params
@tudor-malene tudor-malene - fixed GRAILS-8963
 - implemented GRAILS-9804
 - added better error reporting when calling named query with wrong no of named criteria params
d4394e8
@jeffbrown

IllegalArgumentExceptions are generally used to indicate that an argument value is inappropriate. I don't think we should use that to indicate that the wrong number of arguments were passed. Normally that results in a MissingMethodException.

Yes, normally it throws a MissingMethodException when the actual Closure call fails, which is not the most intuitive in this case.
This would be an early message to point out the exact problem.
What about an assert ?

@jeffbrown
Owner

I can't tell which pieces of this commit are relevant to GRAILS-8963. Are there any tests for GRAILS-8963? At least some of that was addressed with http://jira.grails.org/browse/GRAILS-7945. I am trying to identify how the commit here expands upon that.

GRAILS-8963 happens because AbstractCallSite , which intercepts the 'list' call of NamedCriteriaProxy , transforms the parameters into a Object[] via ArrayUtil.createArray(arg1, arg2) and then , because of the method signature ( the first parameter is of type Object[]) it gets confused , so the additionalCriteriaClosure ends up as the last element in that array.

One solution would be to change the signature of the method to:
def list(def params, def additionalCriteriaClosures = null)

The other solution ( if you chose not to change the signature ) is to forward the call to that handleParams method, which in turn forwards it to listInternal after removing the closure.

The test to verify this is: void testClosureAfterList() at line 449 in NamedCriteriaTests.

In your pull request the list and listDistinct methods each have an optional argument that is never used.

check out new commit:
b216d0b

@tudor-malene
@tudor-malene
  • to fix GRAILS-8963 , I added 2 signatures for list and listDistinct . Before, CallSite appended the additional criteria closure to the params array.
  • Reverted the changes to support multiple additional closures, and added the toDetachedCriteria method ( see http://jira.grails.org/browse/GRAILS-9804#comment-73961 )
  • removed the IllegalArgumentExcpetion thrown when parameter nr. didn't match with the named params
@jeffbrown
Owner

I haven't thought it all through yet but at a glance I think getting rid of the Object[] param and replacing it with a Map is probably a good thing. As long as we don't stumble across a reason that might be problematic, we should go with that. Well done.

This pull request contains several things glommed together including more than 1 JIRA (see tudor-malene@d4394e8). Can you submit separate pull requests that address distinct issues? That is a pretty simple thing to do. If you need git help, let me know. Having them separate is good for numerous reasons. One reason is if we ever have a problem and need to revert a commit we don't want to also revert other unrelated stuff just because it happens to be in the same commit. Another is that separate commits makes the individual improvements easier to review and doesn't tie the acceptance of one change to the acceptance of another. In this particular case, fixing the PagedResultList thing is really unrelated to the thing related to multiple criteria closures but they are bundled in the same commit.

Another thing is I know you put a good bit of effort into this (thanks!) and then made changes in response to my comments (thanks again!). That is all great but if you are concerned about putting effort into something that may or may not be accepted, a good process for non-trivial changes is to discuss the proposal on the dev list where a consensus can be pursued and then the coding work can be done in response to that. I mention all of that for your benefit, not ours. I feel bad if someone puts a lot of effort into something and it ends up not being accepted.

Thanks again for all of the help.

@tudor-malene

Thanks for the feedback Jeff.
You're right , I should have separated the 2. I got carried away.

On this pull request, I removed all stuff except the PageResultList bug ( which is actually a CallSite bug: see http://jira.grails.org/browse/GRAILS-8963?focusedCommentId=73968#comment-73968 ) .

But I'm not sure how to create a new pull request for the other changes ( the DetachedCriteria stuff ).

Regarding the effort, I was just looking for a way to solve the problem of using a named query as a base for a dynamic report, and thanks to you and Peter, I came to a simpler approach than the one I considered initially, which is great. And while I was at it, I looked into that other issue too.

@jeffbrown
Owner

If you submit a pull request and later add other commits to that branch before the pull request is merged, those commits become part of the pull request. The best way to create the pull requests is to have separate branches in your repo, one for each pull request, and initiate the pull request from those branches. In the github interface when you initiate the pull request you get to dictate which branch the pull request should be associated with. What some folks do is create a branch whose name contains the corresponding JIRA number, or a name that describes whatever the pull request is about. From our end it doesn't really matter what you call the branch. I hope that makes sense.

I am going to go ahead and close this pull request and we will keep an eye out of the new ones.

Thanks again.

@jeffbrown jeffbrown closed this
@tudor-malene

Check out:
#321
and
#322

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 30, 2013
  1. @tudor-malene

    - fixed GRAILS-8963

    tudor-malene authored
     - implemented GRAILS-9804
     - added better error reporting when calling named query with wrong no of named criteria params
Commits on Jan 31, 2013
  1. @graemerocher @tudor-malene

    allow passing of arguments to forked test-app, fixed resuming forked …

    graemerocher authored tudor-malene committed
    …processes and clear out resumes on exit of interactive mode
  2. @graemerocher @tudor-malene

    spock plugin now working in forked test-app

    graemerocher authored tudor-malene committed
  3. @tudor-malene

    - changes after feedback

    tudor-malene authored
  4. @tudor-malene
Commits on Feb 1, 2013
  1. @tudor-malene
This page is out of date. Refresh to see the latest.
View
20 ...-bootstrap/src/main/groovy/org/codehaus/groovy/grails/cli/fork/ForkedGrailsProcess.groovy
@@ -97,7 +97,6 @@ abstract class ForkedGrailsProcess {
// wait for resume indicator
def resumeDir = new File(executionContext.projectWorkDir, resumeIndicatorName)
resumeDir.mkdirs()
- resumeDir.deleteOnExit()
startIdleKiller()
while (resumeDir.exists()) {
sleep(100)
@@ -134,8 +133,9 @@ abstract class ForkedGrailsProcess {
@CompileStatic
- Process fork() {
+ Process fork(Map argsMap = new LinkedHashMap()) {
ExecutionContext executionContext = getExecutionContext()
+ executionContext.argsMap = argsMap
if (reloading) {
discoverAndSetAgent(executionContext)
}
@@ -148,7 +148,7 @@ abstract class ForkedGrailsProcess {
String classpathString = getBoostrapClasspath(executionContext)
List<String> cmd = buildProcessCommand(executionContext, classpathString, true)
- return forkReserveProcess(cmd, executionContext)
+ forkReserveProcess(cmd, executionContext)
}
else {
String classpathString = getBoostrapClasspath(executionContext)
@@ -174,14 +174,19 @@ abstract class ForkedGrailsProcess {
}
@CompileStatic
- protected Process forkReserveProcess(List<String> cmd, ExecutionContext executionContext) {
- final p2 = new ProcessBuilder()
+ protected void forkReserveProcess(List<String> cmd, ExecutionContext executionContext) {
+ final builder = new ProcessBuilder()
.directory(executionContext.getBaseDir())
.redirectErrorStream(false)
.command(cmd)
- .start()
+ Thread.start {
- attachOutputListener(p2, true)
+ sleep 2000
+ final p2 = builder.start()
+
+ attachOutputListener(p2)
+
+ }
}
@CompileStatic
@@ -422,6 +427,7 @@ class ExecutionContext implements Serializable {
String env
File grailsHome
+ Map argsMap
void initialize(BuildSettings settings) {
List<File> isolatedBuildDependencies = buildMinimalIsolatedClasspath(settings)
View
3  ...src/main/groovy/org/codehaus/groovy/grails/cli/fork/testing/ForkedGrailsTestRunner.groovy
@@ -86,6 +86,7 @@ class ForkedGrailsTestRunner extends ForkedGrailsProjectClassExecutor {
GrailsBuildEventListener eventListener = (GrailsBuildEventListener)scriptBinding.getVariable("eventListener")
scriptBinding.setVariable("pluginSettings", pluginSettings)
scriptBinding.setVariable("grailsSettings", buildSettings)
+ eventListener.initialize()
final projectCompiler = forkedClassLoader.loadClass("org.codehaus.groovy.grails.compiler.GrailsProjectCompiler").newInstance(pluginSettings)
final projectPackager = forkedClassLoader.loadClass("org.codehaus.groovy.grails.project.packaging.GrailsProjectPackager").newInstance(projectCompiler, eventListener)
@@ -121,7 +122,7 @@ class ForkedGrailsTestRunner extends ForkedGrailsProjectClassExecutor {
instance.projectPackager.projectCompiler.configureClasspath()
instance.projectPackager.packageApplication()
- instance.runAllTests()
+ instance.runAllTests(executionContext.argsMap)
}
View
8 ...otstrap/src/main/groovy/org/codehaus/groovy/grails/cli/interactive/InteractiveMode.groovy
@@ -94,6 +94,14 @@ class InteractiveMode {
String originalGrailsEnvDefault = System.getProperty(Environment.DEFAULT)
addStatus("Enter a script name to run. Use TAB for completion: ")
+ Runtime.addShutdownHook {
+ final files = settings.projectWorkDir.listFiles()
+ if (files) {
+ final toDelete = files.findAll { File f -> f.name.endsWith("-resume") && f.directory }
+ toDelete*.delete()
+ }
+ }
+
while (interactiveModeActive) {
String scriptName = showPrompt()
if (scriptName == null) {
View
27 ...n/groovy/org/codehaus/groovy/grails/orm/hibernate/cfg/HibernateNamedQueriesBuilder.groovy
@@ -98,7 +98,7 @@ class NamedCriteriaProxy<T> {
queryBuilder?."${propName}" = val
}
- private listInternal(Object[] params, Closure additionalCriteriaClosure, Boolean isDistinct) {
+ private listInternal(Map paramsMap, Closure additionalCriteriaClosure, Boolean isDistinct) {
def listClosure = {
queryBuilder = delegate
invokeCriteriaClosure(additionalCriteriaClosure)
@@ -107,11 +107,6 @@ class NamedCriteriaProxy<T> {
}
}
- def paramsMap
- if (params && params[-1] instanceof Map) {
- paramsMap = params[-1]
- }
-
if (paramsMap) {
domainClass.clazz.createCriteria().list(paramsMap, listClosure)
} else {
@@ -119,20 +114,28 @@ class NamedCriteriaProxy<T> {
}
}
- def list(Object[] params, Closure additionalCriteriaClosure = null) {
- listInternal params, additionalCriteriaClosure, false
+ def list( Map args = Collections.emptyMap(), Closure additionalCriteriaClosure = null) {
+ listInternal args, additionalCriteriaClosure, false
+ }
+ def list( Closure additionalCriteriaClosure) {
+ list (Collections.emptyMap(), additionalCriteriaClosure)
}
- def listDistinct(Object[] params, Closure additionalCriteriaClosure = null) {
- listInternal params, additionalCriteriaClosure, true
+ def listDistinct( Map args = Collections.emptyMap(), Closure additionalCriteriaClosure = null) {
+ listInternal args, additionalCriteriaClosure, true
+ }
+ def listDistinct( Closure additionalCriteriaClosure) {
+ listDistinct (Collections.emptyMap(), additionalCriteriaClosure)
}
def call(Object[] params) {
if (params && params[-1] instanceof Closure) {
def additionalCriteriaClosure = params[-1]
params = params.length > 1 ? params[0..-2] : [:]
+ def listArgs = [:]
if (params) {
- if (params[-1] instanceof Map) {
+ if (params[-1] instanceof Map) { // the last parameter is a map with list settings
+ listArgs = params[-1]
if (params.length > 1) {
namedCriteriaParams = params[0..-2] as Object[]
}
@@ -140,7 +143,7 @@ class NamedCriteriaProxy<T> {
namedCriteriaParams = params
}
}
- list(params, additionalCriteriaClosure)
+ list(listArgs, additionalCriteriaClosure)
}
else {
namedCriteriaParams = params
View
46 ...stence/src/test/groovy/org/codehaus/groovy/grails/orm/hibernate/NamedCriteriaTests.groovy
@@ -319,6 +319,52 @@ class NamedCriteriaTests extends AbstractGrailsHibernateTests {
assertEquals 0, results?.size()
}
+
+ //see GRAILS-8963
+ void testClosureAfterList(){
+ def now = new Date()
+
+ 6.times { cnt ->
+ assert new NamedCriteriaPublication(title: "Some Old Book #${cnt}",
+ datePublished: now - 1000, paperback: true).save(failOnError: true).id
+ assert new NamedCriteriaPublication(title: "Some New Book #${cnt}",
+ datePublished: now, paperback: true).save(failOnError: true).id
+ }
+
+ def results = NamedCriteriaPublication.recentPublications.list(max: 10, offset: 0){
+ eq 'paperback', true
+ }
+
+ assertEquals PagedResultList.class, results.class
+ assertEquals 6, results.size()
+
+ results = NamedCriteriaPublication.recentPublications.list([max: 10, offset: 0]){
+ eq 'paperback', true
+ }
+
+ assertEquals PagedResultList.class, results.class
+ assertEquals 6, results.size()
+
+ //this will be an ArrayList because there are no list arguments
+ results = NamedCriteriaPublication.recentPublications.list(){
+ eq 'paperback', true
+ }
+ assertEquals 6, results.size()
+
+ results = NamedCriteriaPublication.recentPublications.listDistinct(max: 10, offset: 0){
+ eq 'paperback', true
+ }
+
+ assertEquals PagedResultList.class, results.class
+ assertEquals 6, results.size()
+
+ //this will be an arrayList because there are no list arguments
+ results = NamedCriteriaPublication.recentPublications.listDistinct(){
+ eq 'paperback', true
+ }
+ assertEquals 6, results.size()
+ }
+
void testPropertyCapitalization() {
def now = new Date()
[true, false].each { isPaperback ->
View
2  ...est/src/main/groovy/org/codehaus/groovy/grails/test/runner/GrailsProjectTestRunner.groovy
@@ -141,6 +141,8 @@ class GrailsProjectTestRunner extends BaseSettingsApi{
context.setVariable("testReportsDir", this.testReportsDir)
context.setVariable("reportFormats", reportFormats)
context.setVariable("ant", ant)
+ context.setVariable("unitTests", testFeatureDiscovery.unitTests)
+ context.setVariable("integrationTests", testFeatureDiscovery.integrationTests)
testFeatureDiscovery.testExecutionContext = context
}
Something went wrong with that request. Please try again.