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

YAJL: Plugin Adds Empty Parent Key #2132

Closed
sanssecours opened this issue Jul 28, 2018 · 11 comments
Closed

YAJL: Plugin Adds Empty Parent Key #2132

sanssecours opened this issue Jul 28, 2018 · 11 comments

Comments

@sanssecours
Copy link
Member

Steps to Reproduce the Problem

kdb mount config.json user/tests/yajl yajl
kdb ls user/tests/yajl            # Prints nothing βœ…
kdb set user/tests/yajl/key value # Also adds empty parent key 😒
kdb ls user/tests/yajl

Expected Result

The last command should print the following output:

user/tests/yajl/key

. At least YAML CPP handles this situation correctly:

kdb mount config.yaml user/tests/yaml yamlcpp
kdb set user/tests/yaml/key value
kdb ls user/tests/yaml
#> user/tests/yaml/key
kdb get user/tests/yaml
# STDERR: Did not find key 'user/tests/yaml'

.

Actual Result

The command kdb ls user/tests/yajl prints the following output:

user/tests/yajl
user/tests/yajl/key

.

System Information

  • Elektra Version: master
  • Operating System: macOS 10.13.6
@markus2330
Copy link
Contributor

Thank you for reporting the bug!

@PhilippGackstatter
Copy link
Contributor

I think when issue #2129 is resolved I can look into this next.

@markus2330
Copy link
Contributor

Thank you, I assigned you!

@PhilippGackstatter
Copy link
Contributor

I tried to get an understanding of what's going on. So with the above mentioned kdb ls command, elektraYajlGet returns these two keys

user/tests/yajl
user/tests/yajl/key

With YamlCPP on the other hand, yamlcpp::yamlRead returns only one key user/tests/yaml/key.

The files look semantically the same, that is {"key": "value"} for yajl and key: value for yamlcpp. Judging from that, I assume the error lies with elektraYajlGet, meaning it has to filter out the additional key. Is that correct? I guess then it would be enough to remove the parent key if it has no value?

@sanssecours
Copy link
Member Author

Judging from that, I assume the error lies with elektraYajlGet, meaning it has to filter out the additional key. Is that correct?

While filtering should work, I think it would be better to not create any intermediate keys in the first place. For example, if we mount the following file:

{
  "level1": {
    "level2": {
      "level3": "something"
    }
  }
}

at the namespace user, then the plugin should only create the key user/level1/level2/level3 with the value something. It should not create any of the keys:

user
user/level1
user/level1/level2

.

I guess then it would be enough to remove the parent key if it has no value?

I guess so too, but that sounds like a workaround to me. It would probably be enough to fix the problem described in this issue though.

@PhilippGackstatter
Copy link
Contributor

I think it would be better to not create any intermediate keys in the first place

That sounds like a better idea. However from looking at the parsing code, I'm not sure whether this is possible. When parsing level2, user/level1/ must exist in the keyset and be pointed at by the cursor to create the new key user/level1/level2. I tried removing the keys when a map ends, but that wasn't consistently possible and most of all destroyed the rest of the parsing code.

So I created a pull request #2580 which filters the empty keys out, after parsing is done. Looking forward to feedback.

@sanssecours
Copy link
Member Author

That sounds like a better idea. However from looking at the parsing code, I'm not sure whether this is possible.

That might be the case. I do not know the YAJL plugin code at all. Since YAML CPP, Yan LR, YAMBi, YAwn, and Yay PEG do not add any intermediate keys, my guess would be that it should be possible.

When parsing level2, user/level1/ must exist in the keyset and be pointed at by the cursor to create the new key user/level1/level2.

I assume the interface of YAJL is very similar to the one of an ANTLR listener class, meaning that the parser calls specific functions, if it notices a certain element in the JSON file, such as a map or a string. In that case, I think you can handle this situation with code that is similar to Yan LR’s listener class. What Yan LR basically does is keep a temporary stack of keys (parents). The first element in the stack is a copy of the parent key. For each newly started YAML key-value pair it

  1. makes a copy of the top element in the stack,
  2. adds the key of the key-value pair as basename to the copy of the top element,
  3. pushes the updated copy into the stack

. Every time the plugin leaves a key-value pair it drops the last key in the parents stack. The only time the plugin adds the top element of the stack to the key set is, if it finds a key-value pair that contains a string as value. I think the best way to describe this behavior is an example. Let us say the YAJL plugin wants to store the following data:

{
  "level1": {
    "level2": {
      "level3": "something"
    }
  },
  "one": {
    "two": "three"
  }
}

at user/parent. In that case the plugin stores user/parent int the parents stack:

# Stack
user/parent

. Then the plugin parses the JSON file and finds a key-value pair containing the key "level1" and a dictionary as value. This means it is time to copy the top-element of the stack, add the new basename and push the copy back into the stack. Afterwards the stack looks like this:

# Stack
user/parent
user/parent/level1

. Please note, that I reversed the stack (it grows downwards), since in this notation the stack looks more similar to the JSON data.

In the next step YAJL finds another dictionary containing a key-value pair with "level2" as key. Just like in the step before we extend the stack:

# Stack
user/parent
user/parent/level1
user/parent/level1/level2

. The next step looks similar, since YAJL finds another key-value pair, this time with the key "level3":

# Stack
user/parent
user/parent/level1
user/parent/level1/level2
user/parent/level1/level2/level3

. Then the plugin finds the first non-dictionary value ("something"), which means it is time to store the top element of the stack in the key set together with the value something. The key set then looks like this

# Key Set
user/parent/level1/level2/level3: something

. In the next step YAJL leaves the key-value pair "level3": "something". This means it is time to drop the top element of the stack:

# Stack
user/parent
user/parent/level1
user/parent/level1/level2

. After that a } character indicates, that YAJL leaves another key-value pair, and it is time to drop another element from the stack:

# Stack
user/parent
user/parent/level1

. Another closing brace in the next line shows that it is time to drop another element from the stack:

# Stack
user/parent

. Afterwards YAJL finds another key-value pair that contains the key "one" and a dictionary as value. This means it is time to extend the stack again:

# Stack
user/parent
user/parent/one

. YAJL then finds the last key-value pair "two": "three" and extends the stack again:

# Stack
user/parent
user/parent/one
user/parent/one/two

. The non-dictionary value "three" indicates that it is time to store the top element in the key set:

# Key Set
user/parent/level1/level2/level3: something
user/parent/one/two: three

. Afterwards YAJL leaves two key-value pairs, which means it is time to drop two elements from the stack:

# Stack
user/parent

. At the end of the document YAJL destroys the whole stack and returns the final key set:

user/parent/level1/level2/level3: something
user/parent/one/two: three

. I hope the explanation above helps. Just as I said before: I do not know, if the whole procedure is applicable to YAJL, but I would at least assume that something similar should be possible.

@PhilippGackstatter
Copy link
Contributor

Thank you, very clear explanation! But yajl does not use a stack for parsing and instead immediately adds keys to the KeySet and lets the internal cursor point to that element. That doesn't necessarily rule out the possibility of doing that.

In your example, each time a } is encountered, an element is dropped from the stack. So it seemed to me that this function, which parses the end of maps (and arrays) would be a good place to drop the empty keys.

static int elektraYajlParseEnd (void * ctx)
{
KeySet * ks = (KeySet *) ctx;
Key * currentKey = ksCurrent (ks);
Key * lookupKey = keyNew (keyName (currentKey), KEY_END);
keySetBaseName (lookupKey, 0); // remove current baseName
// lets point current to the correct place
Key * foundKey = ksLookup (ks, lookupKey, 0);
#ifdef HAVE_LOGGER
if (foundKey)
{
ELEKTRA_LOG_DEBUG ("%s", keyName (foundKey));
}
else
{
ELEKTRA_LOG_DEBUG ("did not find key!");
}
#else
(void) foundKey; // foundKey is not used, but lookup is needed
#endif
keyDel (lookupKey);
return 1;
}

For instance, with this json

{
  "l1": {
    "l21": 5,
    "l22": 6
  },
  "l11": 3
}

the log (shortened) looks like this

183:elektraYajlParseStartMap: with new key user/tests/yajl/___empty_map
150:elektraYajlParseMapKey: stringValue: l1 currentKey: user/tests/yajl/___empty_map
183:elektraYajlParseStartMap: with new key user/tests/yajl/l1/___empty_map
150:elektraYajlParseMapKey: stringValue: l21 currentKey: user/tests/yajl/l1/___empty_map
107:elektraYajlParseNumber: 5 1
150:elektraYajlParseMapKey: stringValue: l22 currentKey: user/tests/yajl/l1/l21
107:elektraYajlParseNumber: 6 1
205:elektraYajlParseEnd: user/tests/yajl/l1
150:elektraYajlParseMapKey: stringValue: l11 currentKey: user/tests/yajl/l1
107:elektraYajlParseNumber: 3 1
205:elektraYajlParseEnd: user/tests/yajl

So it seems elektraYajlParseEnd: user/tests/yajl/l1 would be the place to remove the printed key.

But when the parser hits the map key l11 the keySet has to point to user/tests/yajl/l1 so that it can overwrite the basename to user/tests/yajl/l11. That means when I hit the end of the map for l1 I can not delete it. So with the way the parser is currently written it cannot possibly know if it can delete a Key when it hits the end of the map, or not.

I'm thinking about changing the parser to adding a basename instead of overwriting it, but I'll have to look into this more later.

For now anyway, I think with the current parser its not possible to easily drop the unneeded keys.

PhilippGackstatter pushed a commit to PhilippGackstatter/libelektra that referenced this issue Apr 7, 2019
- Remove directoryvalue plugin as a yajl
dependency until this is fully implemented
- Issue: ElektraInitiative#2132
@markus2330
Copy link
Contributor

Is this now done?

@PhilippGackstatter Will you continue working on that?

@PhilippGackstatter
Copy link
Contributor

No, PR #2580 has fixed this issue. After fixing it, I thought I'd found a better way, but it was a dead end.

@markus2330
Copy link
Contributor

Thank you for the fast response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants