Disable cascading writes in ambiguous cases #3794

dev2718 · 2021-04-18T17:13:28Z

Proposed Changes

This PR serves, among others, to implement #3742 and #4012.

List of proposed changes:

kdb set, kdb meta-set: (Resolve Inconsistent set/get for cascading keys #401) Only allow writes to the cascading namespace if the lookup succeeds (Otherwise, the operation is ambiguous and therefore aborted).
No more guessing of namespaces in case a cascading key is given (user:, system: for kdb set, spec: for kdb meta-set),
kdb file: Remove namespace guessing
kdb editor/import: Disable the use of cascading names (and the 'validate' strategy operating on cascading keys) entirely to resolve cascading import/export/editor dangerous #2762
kdb export keep behaviour to not break tests/shell/shell_recorder/shell_recorder.sh
kdb get, kdb sget, kdb meta-get, kdb set, kdb meta-set: Use a cascading key for parentKey to solve kdb set user/ and -N user should behave the same, -f for old behavior #2561.
Introduce -f flag to kdb set, kdb meta-set to use old behaviour (bypass validation)
Implement proposal: remove -N from tools #4012
Update numerous tests to comply with changes.
Update documentation of kdb manpages.
Add new subsection on cascading writes to elektra-cascading.md.

Basics

These points need to be fulfilled for every PR:

Short descriptions of your changes are in the release notes
(added as entry in doc/news/_preparation_next_release.md which
contains _(my name)_)
Please always add something to the release notes.
Details of what you changed are in commit messages
(first line should have module: short statement syntax)
References to issues, e.g. close #X, are in the commit messages.
The buildservers are happy. If not, fix in this order:
- add a line in doc/news/_preparation_next_release.md
- reformat the code with scripts/dev/reformat-all
- make all unit tests pass
- fix all memleaks
The PR is rebased with current master.

If you have any troubles fulfilling these criteria, please write
about the trouble as comment in the PR. We will help you.
But we cannot accept PRs that do not fulfill the basics.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

I added unit tests for my code
I fully described what my PR does in the documentation
(not in the PR description)
I fixed all affected documentation
I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
I updated all meta data (e.g. README.md of plugins and METADATA.ini)
I mentioned every code not directly written by me in THIRD-PARTY-LICENSES

Review

Reviewers will usually check the following:

Documentation is introductory, concise, good to read and describes everything what the PR does
Examples are well chosen and understandable
Code is conforming to our Coding Guidelines
APIs are conforming to our Design Guidelines
Code is consistent to our Design Decisions

Labels

If you are already Elektra developer:

Add the "work in progress" label if you do not want the PR to be reviewed yet.
Add the "ready to merge" label if the basics are fulfilled and you also
say that everything is ready to be merged.

mpranj · 2021-04-19T12:02:23Z

Thank you so much for working on this!

Is the plan here to fully implement the proposal from #3742?

markus2330 · 2021-04-19T17:53:06Z

Thank you for creating the PR!

Did you already consider that cascading writes are okay if -N is given? Please also update docu&test cases.

dev2718 · 2021-04-20T12:39:55Z

As of ff85a90 -N is considered.
The old behaviour of guessing the flag -N if not supplied as either "user:" or "system:" is removed in this commit.

Cmdline::createKey will now translate a cascading name to a "namespaced" name if the namespace flag is provided.

All code segments in kdb using the flag have been updated:

kdb editor/import: abort when -N is needed but not supplied as guessing was removed
kdb file: removed behaviour now implemented in Cmdline::createKey

I plan to update documentation and testing once we have settled on the new behaviour.

markus2330

Thank you for the improvements! Docu&test cases are still missing which makes it hard to know what the intended behavior is.

src/tools/kdb/editor.cpp

src/tools/kdb/import.cpp

markus2330 · 2021-07-03T13:20:50Z

I am afraid something went wrong in the rebase. One way to resolve it is to create a new branch, cherry-pick the commits and create a new PR.

dev2718 · 2021-07-04T21:18:35Z

I am afraid something went wrong in the rebase. One way to resolve it is to create a new branch, cherry-pick the commits and create a new PR.

The rebase is fixed now.

markus2330

This PR is starting to look great! 🚀

doc/help/kdb-editor.md

markus2330 · 2021-08-24T04:17:42Z

doc/help/kdb-rm.md

@@ -11,6 +11,10 @@ Note that when using the `-r` flag, not only the key directly at `path` will be

 This command removes key(s) from the Key database.

+## LIMITATIONS
+
+Cascading keys (i.e. keys that start with `/`) are only allowed using the `-f` flag.


Why? And what is the behavior? I would expect it removes any key in any namespace. What is ambiguous here?

I would also avoid giving -f so different meanings: bypassing spec: validation (which also makes sense for kdb-rm) and allowing cascading writes. What if someone does not want to bypass validation but would like to use cascading?

My rationale was to implement #3742 (I interpreted this to mean that all cascading operations on kdb rm were meant to be disabled, as not otherwise specified).
The -f option was intended to keep the old behavior available, but of course this is not an optimal solution.
I will revert it back.

Yes, I think in non-ambiguous cases we can simply remove the key(s). -f should be only for disabling the spec validation.

The remaining problem is that repeated executions of kdb rm -r /something might remove several keys. Please think about it if we really want the same semantics as used in kdb set. How is the behavior in FUSE when you remove a file that is "backed up" by several keys (in several namespaces)?

The old behavior is back.

Although it is not an advisable strategy to do that, I think it is better to not introduce a new special case here. and keep things consistent with the other commands.

In the FUSE tool this case does not occur, since the cascading namespace is mounted read-only (This was decided because a file system that only supports writing partially (due to the ambiguity of creating keys in the cascading namespace) is a mess regarding compatibility with tools and usability for users).

doc/help/kdb-import.md

markus2330 · 2021-08-24T04:18:57Z

doc/help/kdb-meta-get.md

@@ -42,6 +42,8 @@ This command will return the following values as an exit status:<br>
  Explain what is happening. Prints additional information in case of errors/warnings.
 - `-d`, `--debug`:
  Give debug information. Prints additional debug information in case of errors/warnings.
+- `-N`, `--namespace=NS`:
+  Use the specified namespace in case the provided key does not already have a namespace.


What if the namespace is provided twice? (I would expect an error from this docu.)

For a case like kdb meta-get -N system user:/key metakey I opted to use the namespace of the key, since this is how kdb set worked in the past, and throwing an error (while still being consistent with kdb set) leads to e.g. testcase check_distribution.sh breaking.

What do you think is the solution?

The proper solution probably is to completely get rid of -N as we actually want it to be synonymous to add the namespace to the key. I proposed this in #4012

The proposal is now fully implemented.

doc/news/_preparation_next_release.md

doc/tutorials/cascading.md

doc/tutorials/cmerge.md

…behaviour with new shell function prepend_namespace_if_not_present

… namespace" This reverts commit 5ac97e1.

Co-authored-by: Markus Raab <markus2330@users.noreply.github.com>

…lease.md

mpranj · 2021-09-04T15:03:32Z

The manpages are not fully re-generated. The simplest is to apply the patch from the failed CI check: https://cirrus-ci.com/task/5640107244912640

dev2718 · 2021-09-04T15:13:02Z

The manpages are not fully re-generated. The simplest is to apply the patch from the failed CI check: https://cirrus-ci.com/task/5640107244912640

Thank you for the hint; Done.

mpranj

Awesome work! 🚀

doc/help/kdb-meta-set.md

doc/help/kdb-set.md

mpranj · 2021-09-05T10:09:05Z

Thank you!

dev2718 mentioned this pull request Apr 18, 2021

Implement fuse-prototype including context-mocking and README.md #3657

Merged

20 tasks

mpranj added the work in progress label Apr 19, 2021

mpranj added this to the 0.9.6 milestone Apr 19, 2021

dev2718 requested a review from markus2330 April 27, 2021 18:41

dev2718 added the needs review please review this PR label Apr 27, 2021

markus2330 suggested changes Apr 29, 2021

View reviewed changes

src/tools/kdb/editor.cpp Outdated Show resolved Hide resolved

src/tools/kdb/import.cpp Outdated Show resolved Hide resolved

This was referenced Apr 29, 2021

fuse Docker container broken #3817

Closed

fuse native execution in Debian #3818

Closed

markus2330 linked an issue May 2, 2021 that may be closed by this pull request

kdb spec-mount incompatibilities #2760

Closed

mpranj modified the milestones: 0.9.6, 0.9.7 Jun 7, 2021

markus2330 linked an issue Jun 23, 2021 that may be closed by this pull request

kdb rm /..: removes spec key #2761

Closed

dev2718 force-pushed the kdb_cascading_writes branch 2 times, most recently from 39a0f05 to 629bf43 Compare July 4, 2021 21:16

mpranj modified the milestones: 0.9.7, 0.9.* Jul 8, 2021

dev2718 force-pushed the kdb_cascading_writes branch from 5ac510e to 8187098 Compare August 18, 2021 18:59

dev2718 requested a review from markus2330 August 19, 2021 14:58

markus2330 reviewed Aug 24, 2021

View reviewed changes

dev2718 force-pushed the kdb_cascading_writes branch 2 times, most recently from 567461c to e122940 Compare August 29, 2021 12:41

This was referenced Aug 29, 2021

Restyle Disable cascading writes in ambiguous cases #4013

Closed

Restyle Disable cascading writes in ambiguous cases #4014

Closed

Restyle Disable cascading writes in ambiguous cases #4016

Closed

dev2718 mentioned this pull request Aug 31, 2021

proposal: remove -N from tools #4012

Closed

dev2718 and others added 15 commits September 4, 2021 15:37

update check_distribution, check_resolver testcases by emulating old …

cf66e5b

…behaviour with new shell function prepend_namespace_if_not_present

(ElektraInitiative#4012) update manpages: remove -N/--namespace option

ec01273

Restyled by shfmt

0fe158d

(ElektraInitiative#4012) update releasenotes

4c6f483

remove debugging statement

82e6b61

update generated manpages

42a754a

(ElektraInitiative#4012) add option -S/--no-separators to kdb namespace

d58f14b

update man pages; delete chache file

fd21d8d

reformat cpp

c83f9ca

Revert "(ElektraInitiative#4012) add option -S/--no-separators to kdb…

08da162

… namespace" This reverts commit 5ac97e1.

remove kdb dirkey

b98c842

Apply suggestions from code review (doc/wording)

6713e6b

Co-authored-by: Markus Raab <markus2330@users.noreply.github.com>

update glossary with dirname, remove dirkey from _preparation_next_re…

88ab36d

…lease.md

remove remaining mentions of dirkey

afa8bfc

update generated manpages

36901a7

dev2718 force-pushed the kdb_cascading_writes branch from 2b68dbb to 36901a7 Compare September 4, 2021 13:37

apply man.patch to fix generated man pages

8ba6073

mpranj approved these changes Sep 4, 2021

View reviewed changes

doc/help/kdb-meta-set.md Outdated Show resolved Hide resolved

doc/help/kdb-set.md Outdated Show resolved Hide resolved

Apply suggestions from code review

a500b30

mpranj modified the milestones: 0.9.*, 0.9.8 Sep 4, 2021

mpranj added ready to merge and removed work in progress needs review please review this PR labels Sep 4, 2021

mpranj added 2 commits September 4, 2021 22:08

doc: regenerate man pages

ea9fecf

Merge branch 'master' into kdb_cascading_writes

1f412a2

mpranj merged commit 8b36ff6 into ElektraInitiative:master Sep 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable cascading writes in ambiguous cases #3794

Disable cascading writes in ambiguous cases #3794

dev2718 commented Apr 18, 2021 •

edited

Loading

mpranj commented Apr 19, 2021

markus2330 commented Apr 19, 2021

dev2718 commented Apr 20, 2021

markus2330 left a comment

markus2330 commented Jul 3, 2021

dev2718 commented Jul 4, 2021

markus2330 left a comment

markus2330 Aug 24, 2021

dev2718 Aug 24, 2021

markus2330 Aug 29, 2021

dev2718 Aug 31, 2021

markus2330 Aug 24, 2021

dev2718 Aug 24, 2021

markus2330 Aug 29, 2021

dev2718 Aug 31, 2021

mpranj commented Sep 4, 2021

dev2718 commented Sep 4, 2021

mpranj left a comment

mpranj commented Sep 5, 2021

Disable cascading writes in ambiguous cases #3794

Disable cascading writes in ambiguous cases #3794

Conversation

dev2718 commented Apr 18, 2021 • edited Loading

Proposed Changes

Basics

Checklist

Review

Labels

mpranj commented Apr 19, 2021

markus2330 commented Apr 19, 2021

dev2718 commented Apr 20, 2021

markus2330 left a comment

Choose a reason for hiding this comment

markus2330 commented Jul 3, 2021

dev2718 commented Jul 4, 2021

markus2330 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpranj commented Sep 4, 2021

dev2718 commented Sep 4, 2021

mpranj left a comment

Choose a reason for hiding this comment

mpranj commented Sep 5, 2021

dev2718 commented Apr 18, 2021 •

edited

Loading