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

Can't get or push any data with "/" in the key. #75

Closed
iainvm opened this issue Mar 15, 2019 · 7 comments
Closed

Can't get or push any data with "/" in the key. #75

iainvm opened this issue Mar 15, 2019 · 7 comments
Labels

Comments

@iainvm
Copy link

iainvm commented Mar 15, 2019

Example:

Code

let data = {
    "norm_key": "data",
    "bad/key":"other data"
}

db.push("normal_key", data);
db.push("bad/key", data);

DB

{
    "normal_key": {
        "norm_key": "data",
        "bad/key":"other data"
    },
    "bad": {
        "key": {
            "norm_key": "data",
            "bad/key":"other data"
        }
    }
}

So you can create a key that has a / in it, but you can push data that does. This makes the "other data" unreachable, since by entering the key /bad/key/bad/key you'll search for

{
    "bad": {
        "key": {
            "bad": {
                "key":"other data"
            }
        }
    }
}

That doesn't exist

@iainvm
Copy link
Author

iainvm commented Mar 15, 2019

datapaths and data are treated differently.

@Belphemur Belphemur added the bug label Mar 15, 2019
@Belphemur
Copy link
Owner

I don't see an easy way to resolve this without having to check the key of the object that are push. This could have a huge impact on the performance depending on of the depth and size of the object.

I'll put this as a known issue in the readme.

@iainvm
Copy link
Author

iainvm commented Mar 15, 2019

Since searching the object that is pushed has performance issues, and is a pain, you could make a way of escaping the "/" in the key, so that it can be resolved and found.

Naive approach, have the key needing escaping. So if there is a "//" (e.g. "bad//key") it will actually include the / in the keyname (i.e. "bad/key"). The only drawback to this is searching for a empty string key. Though that seems more unlikely than a key with a "/" in it.

@Belphemur
Copy link
Owner

I see your point, but since the code work by first splitting each part using the slash (/) as glue, it will need a good refactor of the code which I'm not really keen on doing.

It would be easier for the dev to not use the "/" as part of their object key name since it's used as key separator by JsonDB.

@iainvm
Copy link
Author

iainvm commented Mar 15, 2019

You process the key from a string to an array by splitting on the "/", the method could be overloaded to accept an array outright, giving some heavy lifting to the user of JsonDB but more usability. So allowing them to pass in an already split data path. This would require little to no refactoring.

@iainvm
Copy link
Author

iainvm commented Mar 15, 2019

If this is going to be a known bug, the README.md should be updated to reflect it.

@Belphemur
Copy link
Owner

I've decided to do 2 things:

  1. Add to the readme the limitation
  2. Let the user choose the separator see Let the user choose the separator to use for keys #76

You'll have control on what to use and it not conflicting with your own variable name.

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

No branches or pull requests

2 participants