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

Self-Referential Substitutions don't work #105

Closed
troizky opened this issue Apr 24, 2018 · 11 comments
Closed

Self-Referential Substitutions don't work #105

troizky opened this issue Apr 24, 2018 · 11 comments

Comments

@troizky
Copy link
Contributor

troizky commented Apr 24, 2018

I'm using the example from documentation: https://github.com/lightbend/config/blob/master/HOCON.md#examples-of-self-referential-substitutions

A self-reference resolves to the value "below" even if it's part of a path expression. So for example:

foo : { a : { c : 1 } }
foo : ${foo.a}
foo : { a : 2 }

Here, ${foo.a} would refer to { c : 1 } rather than 2 and so the final merge would be { a : 2, c : 1 }.
Recall that for a field to be self-referential, it must have a substitution or value concatenation as its value. If a field has an object or array value, for example, then it is not self-referential even if there is a reference to the field itself inside that object or array.

And I'm getting crash:
terminate called after throwing an instance of 'hocon::bug_or_broken_exception' what(): resolve_substitutions() did not give us a resolved object
Also I'm getting crash in cases below:

a = {aa = abba}
a.aa = mama
b = ${a} { bb = mia }
# Everything below is failing
b.aa = mea
b.bb = culpa
b = { aa = mea, bb = culpa }
c = ${b.aa}
c = ${b.bb}

Looks like this library implementation doesn't follow the HOCON documentation...

@MikaelSmith
Copy link
Contributor

The implementation is incomplete. I would appreciate any help finishing it for those cases.

@troizky
Copy link
Contributor Author

troizky commented May 4, 2018

Even the simplest testcase does not work:

  "a": {
    "b": 27
  },
  "obj": ${a}
  "val": ${obj.b}

Maybe this one is easier to fix?

@MikaelSmith
Copy link
Contributor

That is likely HC-73. I'll take a quick look to see if I can figure out what's wrong, but don't have a ton of time to spend on it before I'm on vacation for 2 weeks.

@MikaelSmith
Copy link
Contributor

This library was implemented to closely mirror the structure of https://github.com/lightbend/config. So tracing the exception I get with your simple test case back to https://github.com/puppetlabs/cpp-hocon/blob/master/lib/src/values/config_reference.cc#L75 and comparing to https://github.com/lightbend/config/blob/master/config/src/main/java/com/typesafe/config/impl/ConfigReference.java#L70, it looks like the fallback code at https://github.com/puppetlabs/cpp-hocon/blob/master/lib/src/values/config_reference.cc#L61 wasn't implemented. I'll try that out.

@MikaelSmith
Copy link
Contributor

MikaelSmith commented May 4, 2018

It looks like this might have actually been fixed recently in config v1.3.2. So there's some work to incorporate the fixes added. Update: I might be jumping to conclusions here; but this is the area where a fix will probably be needed. If it's not a fix in config 1.3.2, then the lookup_subst is failing somehow.

@troizky
Copy link
Contributor Author

troizky commented May 15, 2018

I've found bug in https://github.com/puppetlabs/cpp-hocon/blob/master/lib/src/values/simple_config_object.cc#L32, must be checking for non-empty.
Corresponding line in Java project: https://github.com/lightbend/config/blob/master/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java#L364
Unfortunately 2 test cases are failing now...

@troizky
Copy link
Contributor Author

troizky commented May 17, 2018

Also found that method config_delayed_merge_object::resolve_substitutions was not implemented like here: https://github.com/lightbend/config/blob/master/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java#L53

After fixing that I'm getting an exception 'should not be reached (merging non-simple_config_object)' in simple_config_object::merged_with_object on parsing following config:

    a = {aa = mama, bb = mia}
    b = ${a}
    b.aa = mea
    # below is failing
    b.bb = culpa

@troizky
Copy link
Contributor Author

troizky commented May 17, 2018

@troizky
Copy link
Contributor Author

troizky commented May 17, 2018

Here is my suggested patch:
cpp-hocon.patch.gz

@troizky
Copy link
Contributor Author

troizky commented May 17, 2018

Open Merge Request #106

@MikaelSmith
Copy link
Contributor

Thanks for digging into this! I'll work on reviewing the PRs.

MikaelSmith pushed a commit that referenced this issue May 18, 2018
Fixes #96, #105.

* Fix simple_config_object.cc

Must be checking for non-empty in accordance to line in Java project: https://github.com/lightbend/config/blob/master/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java#L364

* Fix config_delayed_merge_object.cc, config_delayed_merge_object.hpp

The method config_delayed_merge_object::resolve_substitutions must be implemented like here: https://github.com/lightbend/config/blob/master/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java#L53
Also the class config_delayed_merge_object must inherit from class unmergeable like here: https://github.com/lightbend/config/blob/master/config/src/main/java/com/typesafe/config/impl/ConfigDelayedMergeObject.java#L21

* Fix config_substitution_test.cc

Fix expected results of tests.

* Fix config_delayed_merge_object.cc

Include definition of class resolve_result
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

2 participants