Issue#1243 configurations with unimplemented environments by JonMidhir · Pull Request #1244 · sinatra/sinatra

JonMidhir · 2017-01-28T18:27:00Z

Fixes unpredictable behaviour from Sinatra::ConfigFile detailed in #1243.

Loads settings for the current environment regardless of other environments defined in the configuration file. Previously settings failed for all environments unless all in the file were implemented in the application.
Removes the restriction that settings can only be inherited from other environments, so that this is possible:

default: &default
  foo: bar

development:
  <<: *default

production:
  <<: *default
  foo: baz

test:
  <<: *default

Amends source documentation to represent new behaviour.
Includes tests for the new behaviour.
Backwards compatible with existing settings implementations.

olleolleolle · 2017-09-09T06:55:48Z

-      if hash.respond_to? :keys and hash.keys.all? { |k| environments.include? k.to_s }
-        hash = hash[environment.to_s] || hash[environment.to_sym]
-      end
+    # Given a +hash+ with some application configuration, returns the settings


This word, +hash+ refers to the new-renamed parameter (now called config), so the API docs generation will miss this annotation.

olleolleolle · 2017-09-09T07:01:12Z

    end
+
+    # Returns true if supplied with a hash that has any recognized
+    # +environments+ in it's root keys.


Grammar: it's => its

JonMidhir · 2017-10-31T20:06:07Z

Issues by @olleolleolle fixed & branch rebased.

olleolleolle · 2017-10-31T20:59:24Z

@zzak Is this ready for re-review? I believe so.

zzak · 2017-11-01T01:41:07Z

+
+    # Returns a hash that can be accessed by both strings and symbols, when
+    # supplied with a hash with string keys.
+    def with_indifferent_access(hash)


Do we need Yet Another Indifferent Hash implementation? Can't we use the one in core?

I think this PR precedes the work to build out a full IndifferentHash implementation. I can definitely look at switching to that today.

JonMidhir · 2017-11-07T10:08:45Z

@zzak think this is ready for review. Refactored a good bit and it uses IndifferentHash from core.

olleolleolle · 2017-11-07T10:22:22Z

@JonMidhir Could you be using YAML.safe_load(your_thing)? (This is me asking, not telling.)

That becomes a SafeYAML.load(your_thing) and constrains to the types allowed by the schemas. (That is, white-listed according to a pre-defined list of data-types.)

JonMidhir · 2017-11-07T10:41:55Z

@olleolleolle I'd prefer .safe_load too but it seems to ignore aliased environments then. There may be a way to use it though, let me have a look.

JonMidhir · 2017-11-08T00:35:27Z

OK If we use YAML.safe_load we can allow aliases but we're going to limit the number of types that can be deserialized.

That's bound to break some existing configurations for users. ERB templates that produce Dates, for example, will be cast to Strings unpredictably or error out with Psych::DisallowedClass.

I'm inclined to leave it as-is for now. What do you think @olleolleolle?

zzak · 2017-11-08T03:40:01Z

Re: safe_load, I think it's fine to leave it. It would be a potentially big behavior change and supposing that people know the contents of the config files they are loading.

JonMidhir · 2018-03-06T14:24:36Z

Looks like this is due to be merged soon. Need it rebased?

namusyaka

Sorry for the late reply. As you said, this should be rebased. And then, I pointed a problem about this change, please take a look at it?

namusyaka · 2018-03-06T14:50:11Z

-            end
+            yaml = YAML.load(document)
+            config = config_for_env(yaml)
+            config.each_pair { |key, value| set(key, value) }


You shouldn't remove the conditional expressions because the following unintentional behavior will happen.

# config.yml development: raise_errors: "foo"

require 'sinatra' require 'sinatra/config_file' config_file './config.yml' get '/' do settings.raise_errors.to_s #=> expected "false", but got "foo" end

I think that it isn't desirable to automatically overwrite settings defined by default by config file settings.

@namusyaka Running this against both master and this branch I notice the default setting being overwritten. I don't think the original conditional defends against this behaviour. I can't actually think of a condition where the original would be met at all.

However, I can add this behaviour if you'd prefer?

Hmm, makes sense.
It seems that the sample I showed was wrong.
However, we must grasp the condition that the original conditional expression was trying to protect.

Welcome if you can look it up.
Then, if the original conditional expression is meaningful, we have to add a test on that matter.

I have the case where this would fail. If the value is being set to nil and it's set using this style, the conditional will be met and setting ignored:

# config.yml raise_errors: development: # app.rb settings.raise_errors #=> expected nil, got false

However in both other styles the conditional is not met, and the default value is overwritten:

# config.yml raise_errors: # app.rb settings.raise_errors #=> expected nil, got nil # config.yml development: raise_errors: # app.rb settings.raise_errors #=> expected nil, got nil

If we're changing the behaviour anyway do we really want to stop people from overriding default values using the config file? It seems reasonable that it should be possible to do so.

If not we can exclude all cases or have it function exactly as it did before.

It's no problem, I think.
I've reviewed this change to a certain extent, but I judged that it is almost no problem.

JonMidhir · 2018-03-08T09:30:39Z

Sure thing, I can see the missing conditional. I'll fix it tonight.

namusyaka

Thanks for clarification. lgtm.
Please squash commits.

namusyaka · 2018-03-18T12:54:12Z

-            end
+            yaml = YAML.load(document)
+            config = config_for_env(yaml)
+            config.each_pair { |key, value| set(key, value) }


It's no problem, I think.
I've reviewed this change to a certain extent, but I judged that it is almost no problem.

…ironments in settings file.

JonMidhir · 2018-03-20T10:44:10Z

Done 👍

namusyaka · 2018-03-22T15:14:53Z

@JonMidhir Thank you

## 2.0.4 / 2018-09-15 * Don't blow up when passing frozen string to `send_file` disposition [#1137](sinatra/sinatra#1137) by Andrew Selder * Fix ubygems LoadError [#1436](sinatra/sinatra#1436) by Pavel Rosick�«ò * Unescape regex captures [#1446](sinatra/sinatra#1446) by Jordan Owens * Slight performance improvements for IndifferentHash [#1427](sinatra/sinatra#1427) by Mike Pastore * Improve development support and documentation and source code by Will Yang, Jake Craige, Grey Baker and Guilherme Goettems Schneider ## 2.0.3 / 2018-06-09 * Fix the backports gem regression [#1442](sinatra/sinatra#1442) by Marc-Andr�«± Lafortune ## 2.0.2 / 2018-06-05 * Escape invalid query parameters [#1432](sinatra/sinatra#1432) by Kunpei Sakai * The patch fixes [CVE-2018-11627](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-11627). * Fix undefined method error for `Sinatra::RequiredParams` with hash key [#1431](sinatra/sinatra#1431) by Arpit Chauhan * Add xml content-types to valid html_types for Rack::Protection [#1413](sinatra/sinatra#1413) by Reenan Arbitrario * Encode route parameters using :default_encoding setting [#1412](sinatra/sinatra#1412) by Brian m. Carlson * Fix unpredictable behaviour from Sinatra::ConfigFile [#1244](sinatra/sinatra#1244) by John Hope * Add Sinatra::IndifferentHash#slice [#1405](sinatra/sinatra#1405) by Shota Iguchi * Remove status code 205 from drop body response [#1398](sinatra/sinatra#1398) by Shota Iguchi * Ignore empty captures from params [#1390](sinatra/sinatra#1390) by Shota Iguchi * Improve development support and documentation and source code by Zp Yuan, Andreas Finger, Olle Jonsson, Shota Iguchi, Nikita Bulai and Joshua O'Brien

zzak added the feedback label Jan 30, 2017

JonMidhir force-pushed the issue#1243-configurations_with_unimplemented_environments branch from 9999791 to 8894a8f Compare March 7, 2017 10:29

zzak added the sinatra-contrib label Mar 19, 2017

olleolleolle reviewed Sep 9, 2017

View reviewed changes

JonMidhir force-pushed the issue#1243-configurations_with_unimplemented_environments branch from 8894a8f to 3ba8697 Compare October 31, 2017 19:40

zzak reviewed Nov 1, 2017

View reviewed changes

JonMidhir force-pushed the issue#1243-configurations_with_unimplemented_environments branch from cf36257 to d670cea Compare November 6, 2017 23:27

namusyaka added this to the v2.0.2 milestone Feb 19, 2018

namusyaka requested changes Mar 6, 2018

View reviewed changes

JonMidhir force-pushed the issue#1243-configurations_with_unimplemented_environments branch from d670cea to 9426aa0 Compare March 8, 2018 11:40

namusyaka approved these changes Mar 18, 2018

View reviewed changes

JonMidhir force-pushed the issue#1243-configurations_with_unimplemented_environments branch from 9426aa0 to 5043030 Compare March 20, 2018 09:48

Load current environment settings regardless of support for other env…

dac5160

…ironments in settings file.

JonMidhir force-pushed the issue#1243-configurations_with_unimplemented_environments branch from 5043030 to dac5160 Compare March 20, 2018 09:53

namusyaka merged commit 44660b9 into sinatra:master Mar 22, 2018

jkowens mentioned this pull request Mar 19, 2020

ConfigFile: settings.foo fails when settings file contains non-implemented environments #1243

Closed

dependabot Bot mentioned this pull request Mar 18, 2021

Bump sinatra from 2.0.1 to 2.0.2 cdhenry/four-burner-event-organizer#2

Closed

Conversation

JonMidhir commented Jan 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonMidhir commented Oct 31, 2017

Uh oh!

olleolleolle commented Oct 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonMidhir commented Nov 7, 2017

Uh oh!

olleolleolle commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonMidhir commented Nov 7, 2017

Uh oh!

JonMidhir commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zzak commented Nov 8, 2017

Uh oh!

JonMidhir commented Mar 6, 2018

Uh oh!

namusyaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonMidhir commented Mar 8, 2018

Uh oh!

namusyaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonMidhir commented Mar 20, 2018

Uh oh!

namusyaka commented Mar 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

olleolleolle commented Nov 7, 2017 •

edited

Loading

JonMidhir commented Nov 8, 2017 •

edited

Loading