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

#256 - Process substitution overrides in order #257

Merged
merged 1 commit into from
Jul 11, 2021

Conversation

JettJones
Copy link
Contributor

@JettJones JettJones commented Mar 13, 2021

Fixing #256 - and a small radius around it.

The Scenario

Using hocon in a larger system where this came up.

keys: { something: "${lookup}-${name}" }

overridden by:
keys: { something: "${lookup}-legacy-name" }

During substitution & replacement, the following happened:

Starting queue had these substitutions: [${lookup}(parent), ${name}(parent), ${lookup}(child)]

  1. Process ${lookup}(parent), the result value still had -${name}, so this substitution stays in the set to re-process, and enqueues ${name}(2) at the end of the list.
  2. Process ${name}(parent) - this produces a static result string, and is removed.
  3. Process ${lookup}(child) - this produces a static result string, and is removed.
    (that's all the substitutions, so the loop restarts)
  4. Process ${lookup}(parent) - overwrites step 3. No overrides remain, so is removed.
  5. Process ${name}(2) - also overwrites step 3. No overrides remain, so is removed.

Proposed Fix:

When looking at a substitution, first check if it has a parent override, and if that parent is active in the substitution list. Wait for all parents to complete before processing the substitution.

This caused two test failures:

  • g: ${?missing1} ${?missing2} would be alternatively added or removed from config as the substitutions processed. This diff changes _do_substitute to only write/add a value if the value is not None.
  • At step 3 above, if lookup is not processed, this is detected as a config loop, because the set of substitutions to evaluate is the same after the loop (${name} is added once and removed once). To get around this, I added a second check to avoid re-queueing substitutions that are already being processed.

Testing

  • Added new test case
  • Passed all tests (except NamedTemporaryFile)
  • NamedTemporaryFile

^ NamedTemporaryFile does not work on Windows (python.org) - I'll try to kick the tires in docker if there's traction for landing this.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 95.377% when pulling 84344e0 on JettJones:jettjones/bug256 into 07758b8 on chimpler:master.

5 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 95.377% when pulling 84344e0 on JettJones:jettjones/bug256 into 07758b8 on chimpler:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 95.377% when pulling 84344e0 on JettJones:jettjones/bug256 into 07758b8 on chimpler:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 95.377% when pulling 84344e0 on JettJones:jettjones/bug256 into 07758b8 on chimpler:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 95.377% when pulling 84344e0 on JettJones:jettjones/bug256 into 07758b8 on chimpler:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 95.377% when pulling 84344e0 on JettJones:jettjones/bug256 into 07758b8 on chimpler:master.

@coveralls
Copy link

coveralls commented Mar 13, 2021

Coverage Status

Coverage increased (+0.8%) to 96.181% when pulling 84344e0 on JettJones:jettjones/bug256 into 07758b8 on chimpler:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 95.377% when pulling 84344e0 on JettJones:jettjones/bug256 into 07758b8 on chimpler:master.

@darthbear
Copy link
Member

Thank you @JettJones for the fix!

@darthbear darthbear merged commit 32f5f57 into chimpler:master Jul 11, 2021
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

Successfully merging this pull request may close these issues.

3 participants