Fix #2999 always include code in error response

Improve xml response
rundeck · Dec 14, 2017 · 889efe0 · 889efe0
1 parent 06c6b65
commit 889efe0
Show file tree

Hide file tree

Showing 2 changed files with 75 additions and 9 deletions.
diff --git a/rundeckapp/grails-app/services/rundeck/services/ApiService.groovy b/rundeckapp/grails-app/services/rundeck/services/ApiService.groovy
@@ -744,9 +744,7 @@ class ApiService {
                 error: true,
                 apiversion: ApiRequestFilters.API_CURRENT_VERSION,
         ]
-        if (code) {
-            result.errorCode=code
-        }
+        result.errorCode = code ?: 'api.error.unknown'
         if (!messages) {
             result.'message'=messageSource.getMessage("api.error.unknown", null, null)
         }
@@ -769,17 +767,16 @@ class ApiService {
         }
         xml.with {
             result(error: "true", apiversion: ApiRequestFilters.API_CURRENT_VERSION) {
-                def errorprops = [:]
-                if (code) {
-                    errorprops = [code: code]
-                }
+                def errorprops = [code: code ?: 'api.error.unknown']
                 delegate.'error'(errorprops) {
                     if (!messages) {
                         delegate.'message'(messageSource.getMessage("api.error.unknown",null,null))
                     }
                     if (messages instanceof List) {
-                        messages.each {
-                            delegate.'message'(it)
+                        delegate.'messages' {
+                            messages.each {
+                                delegate.'message'(it)
+                            }
                         }
                     }else if(messages instanceof Map && messages.code){
                         delegate.'message'(messages.message?:messageSource.getMessage(messages.code, messages.args?messages.args as Object[]:null, null))

diff --git a/rundeckapp/test/unit/rundeck/services/ApiServiceSpec.groovy b/rundeckapp/test/unit/rundeck/services/ApiServiceSpec.groovy
@@ -25,8 +25,11 @@ import grails.test.mixin.TestMixin
 import grails.test.mixin.web.ControllerUnitTestMixin
 import grails.web.JSONBuilder
 import groovy.xml.MarkupBuilder
+import org.codehaus.groovy.grails.plugins.codecs.JSONCodec
+import org.springframework.context.MessageSource
 import rundeck.AuthToken
 import rundeck.User
+import rundeck.filters.ApiRequestFilters
 import spock.lang.Specification
 import spock.lang.Unroll
 
@@ -41,6 +44,9 @@ import java.time.ZoneId
 @TestMixin(ControllerUnitTestMixin)
 @Mock([User, AuthToken])
 class ApiServiceSpec extends Specification {
+    void setup() {
+        mockCodec(JSONCodec)
+    }
     def "renderWrappedFileContents json"(){
         given:
         def builder = new JSONBuilder()
@@ -572,4 +578,67 @@ class ApiServiceSpec extends Specification {
         e.message =~ /Roles are required/
 
     }
+
+    @Unroll
+    def "render error json"() {
+        given:
+        service.messageSource = Mock(MessageSource) {
+            getMessage(_, _, _) >> { it[0] + (it[1] ?: '') }
+        }
+        when:
+        def result = service.renderErrorJson(data, code)
+
+        then:
+        println result
+        result == '{"error":true,"apiversion":' +
+                (ApiRequestFilters.API_CURRENT_VERSION) +
+                        (expectCode ? ',"errorCode":"' + (expectCode) +'"' : '') +
+                        (eMessage ? ',"message":"' + (eMessage) +'"' : '') +
+                        (eMessages ? ',"messages":' + (eMessages) : '') +
+                        '}'
+
+
+        where:
+        code   | data                                 | expectCode          | eMessage            | eMessages
+        null   | [:]                                  | 'api.error.unknown' | 'api.error.unknown' | null
+        'test' | [:]                                  | 'test'              | 'api.error.unknown' | null
+        'test' | ['a', 'b']                           | 'test'              | null                | '["a","b"]'
+        'test' | [code: 'acode']                      | 'test'              | 'acode'             | null
+        'test' | [code: 'acode', message: 'amessage'] | 'test'              | 'amessage'          | null
+        'test' | [code: 'acode', args: ['a', 'b']]    | 'test'              | 'acode[a, b]'       | null
+        'test' | [message: 'amessage']                | 'test'              | 'amessage'          | null
+
+    }
+
+    @Unroll
+    def "render error xml"() {
+        given:
+        service.messageSource = Mock(MessageSource) {
+            getMessage(_, _, _) >> { it[0] + (it[1] ?: '') }
+        }
+        when:
+        def result = service.renderErrorXml(data, code)
+        then:
+        result == """<result error='true' apiversion='${ApiRequestFilters.API_CURRENT_VERSION}'>
+  <error code='${
+            code
+        }'>
+    """+
+                (emessage?("<message>${emessage}</message>"):'')+
+                (elist?("""<messages>\n      ${elist}\n    </messages>"""):'')+
+        """
+  </error>
+</result>"""
+
+        where:
+        code    | data                                 | emessage            | elist
+        'acode' | null                                 | 'api.error.unknown' | null
+        'acode' | [code: 'acode']                      | 'acode'             | null
+        'acode' | [code: 'acode', args: ['a', 'b']]    | 'acode[a, b]'       | null
+        'acode' | [code: 'acode', message: 'zmessage'] | 'zmessage'          | null
+        'acode' | [message: 'amessage']                | 'amessage'          | null
+        'acode' | ['a', 'b']                           | null                | '''<message>a</message>\n      <message>b</message>'''
+
+
+    }
 }