Skip to content
This repository was archived by the owner on Feb 16, 2025. It is now read-only.

cascading import/export/editor dangerous #2762

Closed
markus2330 opened this issue Jun 9, 2019 · 10 comments · Fixed by #3794
Closed

cascading import/export/editor dangerous #2762

markus2330 opened this issue Jun 9, 2019 · 10 comments · Fixed by #3794
Assignees

Comments

@markus2330
Copy link
Contributor

cascading import/export/editor are currently not implemented and should fail until we have implemented it. As solution write to import/export/editor all namespaces separately (even though then validation does not work 😢).

One recent problem: In #2760 the last import removed the spec key.

@kodebach
Copy link
Member

kodebach commented Jun 9, 2019

One recent problem: In #2760 the last import removed the spec key.

That is probably a merging problem. It seems kdb import treats the imported KeySet as a full replacement for the current part of the KDB rooted at the given key. This is not what I would expect from an import command. Instead I would expect that we just insert new keys, overwrite existing ones and leave all other as is.

In a more complicated example with a full LCDproc config, I actually got merging conflicts and import failed.

cascading import/export/editor are currently not implemented and should fail

The only way I see any of these working, is like this:

  • A cascading key is given
  • If -N is given set the parent key namespace to that
  • If we are root set the namespace to system
  • Else set the namespace to user
  • Proceed as if the resulting key was given by the user

That shouldn't be too hard to implement. It already works like this in kdb file.

Anything actually involving some sort of cascading lookup where different namespaces are mixed together would only cause problems. It would also be very intransparent.

@markus2330
Copy link
Contributor Author

That is probably a merging problem. [...] In a more complicated example with a full LCDproc config, I actually got merging conflicts and import failed.

Thank you for the input!

The only way I see any of these working, is like this:

Of course it would be better if it works. Otherwise @Piankero can also simply let it fail when cascading keys are given.

@ghost
Copy link

ghost commented Nov 25, 2019

I am not sure what I should do here as I am just mentioned. Probably with the keyname overhaul this is fixed?

@ghost ghost removed their assignment Nov 25, 2019
@markus2330
Copy link
Contributor Author

@MasterToney can you fix this by simply not allowing cascading keys for import/export?

@MasterToney
Copy link
Contributor

Just to be sure that I understand the task correctly:
Import/export should only allow keys with a namespace otherwise a suiting exception should be thrown.
Examples:
kdb import /sample/key should throw an exception
kdb import user/sample/key should work

Same thing with export?

@markus2330
Copy link
Contributor Author

Good that you ask! You understood correctly but this would be inconsistent to #2561.

So we want following behavior:

kdb import/export/set/editor /sample/key

will write to what is given by -N and print the message which key is used (can be shut off by -q).

kdb import/export/set/editor user/sample/key

you can reject for now (maybe @kodebach or you will add support later after #3115)

Internally, you use the cascading key to kdbGet/kdbSet configuration by default but not the cascading key manipulating the KeySet (here you use namespace + key).

Only if -f is given, you call kdbSet with the namespace (not cascading, then validation is turned off; only for setting operations this makes sense: so set and import).

Maybe do not start with kdb import as import is currently modified in #3260 (the others are not but editor might be later, best ask @Chemin1 about that).

Thank you for picking this up! Please do not hesitate to ask any further questions.

@kodebach
Copy link
Member

you can reject for now (maybe @kodebach or you will add support later after #3115)

The version with namespace shouldn't be much work to implement either.

You just call getNamespace() on the Key to get the namespace. That namespace will be used just like the one given by -N. To get the cascading key you can use k.setName(k.substr(k.find('/'))).

The only decision left is what to do if -N and a key with namespace are used.

@markus2330
Copy link
Contributor Author

The only decision left is what to do if -N and a key with namespace are used.

Exactly, that is why I suggested to reject it now. Unfortunately, there is no way to check if -N was given (or you just see the default). And when -N is used, people will have no problem to migrate to the new namespace : syntax.

@stale
Copy link

stale bot commented Nov 27, 2020

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

@markus2330
Copy link
Contributor Author

To be solved with #3742.

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

Successfully merging a pull request may close this issue.

4 participants