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

Grails 3.0.4: Using both application.yml and application.groovy each with grails.* settings results in broken mimeType handling #9189

Closed
scrain opened this issue Aug 17, 2015 · 10 comments
Assignees
Milestone

Comments

@scrain
Copy link

scrain commented Aug 17, 2015

Sample app can be found in github here

If you have both application.yml and application.groovy files each with properties that start with grails. the content-type response header for generated controllers returns as application/xhtml+xml instead of text\html at which point the browser errors out due to undefined entities.

Note: I discovered this while using the new spring security core plugin, since it generates an application.groovy file with several grails.* properties, but the plugin is not required to recreate the issue.

Additional info: Not sure if this is related, but logging the runtime value of grailsApplication.config.get( 'grails.mime.types' ) shows that the order of the values is effected when an application.groovy file is merged that contains grails. properties.

mimeTypes order when application.groovy contains NO grails. properties

all:*/*,
atom:application/atom+xml,
css:text/css,
csv:text/csv,
form:application/x-www-form-urlencoded,
html:[text/html,application/xhtml+xml],
js:text/javascript,
json:[application/json,text/json],
multipartForm:multipart/form-data,
pdf:application/pdf,
rss:application/rss+xml,
text:text/plain,
hal:[application/hal+json,application/hal+xml],
xml:[text/xml,application/xml],
html[1]:application/xhtml+xml,
xml[1]:application/xml,
hal[0]:application/hal+json,
json[0]:application/json,
hal[1]:application/hal+xml,
html[0]:text/html,
xml[0]:text/xml,
json[1]:text/json

mimeTypes order when application.groovy contains grails. properties

html[1]:application/xhtml+xml,
csv:text/csv,
text:text/plain,
css:text/css,
xml[1]:application/xml,
hal[0]:application/hal+json,
rss:application/rss+xml,
js:text/javascript,
json[0]:application/json,
pdf:application/pdf,
form:application/x-www-form-urlencoded,
atom:application/atom+xml,
hal[1]:application/hal+xml,
html[0]:text/html,
all:*/*,
xml[0]:text/xml,
json[1]:text/json,
multipartForm:multipart/form-data,
html:[text/html,application/xhtml+xml],
json:[application/json,text/json],
hal:[application/hal+json,application/hal+xml],
xml:[text/xml,application/xml] 
@scrain
Copy link
Author

scrain commented Aug 17, 2015

FYI there are a couple of work-arounds to this issue that got me moving again. I settled on option 1 for now since it's more pure grails 3.

  1. Move all grails.* related settings out of application.groovy and into application.yml. I was able to do this for my spring security core settings, but I expect there may be other times when groovy is required and it might not be possible.
  2. If groovy really is required, you can move the mime type settings out of application.yml and into application.groovy.

@jeffscottbrown
Copy link
Member

Thanks for the info Shawn. One of us will look at it right away.

@jeffscottbrown
Copy link
Member

This issue is understood and is a little bit of a pickle because the behavior is being inherited from boot. I think boot expects the app to be using one or the other, but not both config files. We already override boot's yaml loading so we can improve this situation by changing what we are doing in Grails. However, this would be a change that I think is too drastic to introduce in a 3.0.5 release. If we change that, I think it makes more sense to introduce the new behavior in 3.1.

For now I think the thing to do is understand the limitations of the current system and let that guide developers. I think that it is ok to use both application.groovy and application.yml as long as the properties in the 2 files don't share any common roots. For example, if you define grails.foo.bar in application.yml and define app.foo.bar in application.groovy, everything will be fine. A problem will occur if the 2 files try to define any properties that share a common root path. For example, defining grails.foo.bar in one and defining grails.mime.types in another will be problematic.

If we can agree on this then I think the short term thing to do is update the user guide and the longer term thing is to change the behavior in 3.1 so that the 2 files are properly merged instead of one stepping on the other.

@scrain - What do you think?

@graemerocher - What do you think?

Rest of the community - What do you think?

@scrain
Copy link
Author

scrain commented Aug 17, 2015

This is fine with me given the work arounds available.

Should there a be a consideration to change SSC plugin's generation of settings into application.groovy? Not sure if all of its settings can be represented in YAML. Otherwise this will likely bite most users of the plugin.

Other option might be a more pro-active detection of the contention between the two files and an explicit error or warning being output during start-up.

@scrain
Copy link
Author

scrain commented Aug 17, 2015

I glanced at the source... looks like SSC's settings should work in YAML, but it would be best to make that the convention for porting the rest of SS's plugins moving forward if that is the desired approach.

@burtbeckwith
Copy link
Contributor

I'm fine with adding a note in the plugin docs and/or moving the config settings to application.yml, but the real fix is to either merge the two correctly or disallow using both. The initial config settings registered by s2-quickstart will work in either format, but there are settings that won't, e.g. ones where the value is a Closure. There's also a significant amount of flexibility when using Groovy format since you can use loops, logic checks, etc.

@scrain
Copy link
Author

scrain commented Aug 18, 2015

Sorry, I should have been clearer in my comment. I didn't mean for this to be considered the real fix, but more of something that could be done relatively quickly to keep users of the plugin from running into this issue while waiting for the longer term solution.

I figured there was likely some closure based config out there that would make this not completely viable.

@graemerocher graemerocher modified the milestones: grails-3.1, grails-3.0.5 Aug 27, 2015
@kschmit90
Copy link

I am in a situation where using application.groovy is essential because of the need to do conditional logic in the config file for external environment detection.

It seems confusing to even have application.yml, as it appears to remove flexibility within the config, and create the need for two files when there is a situation requiring more than YAML can provide.

This is probably a naive opinion, but this is my experience so far.

@jeffscottbrown
Copy link
Member

It seems confusing to even have application.yml, as it appears to remove flexibility within the config, and create the need for two files when there is a situation requiring more than YAML can provide.

There isn't really a need for two files. You can use 1 or the other if that is your preference. Each offers their own benefits. Most applications manage fine with just application.yml. Some will want to use application.groovy for some things and the bug reported here makes that more tedious than it should be. For those who prefer application.groovy altogether, I don't think there is anything preventing you from using that exclusively.

@graemerocher
Copy link
Member

I found a solution that did not require a major rework (see the above commit), so I am including the fix in 3.0.5

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

No branches or pull requests

5 participants