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

bug fix for verdi node delete #2545

Merged
merged 3 commits into from
Feb 28, 2019
Merged

bug fix for verdi node delete #2545

merged 3 commits into from
Feb 28, 2019

Conversation

zhubonan
Copy link
Contributor

@zhubonan zhubonan commented Feb 27, 2019

Fix a bug such that if no argument is passed to verdi node delete. It will try to delete ALL notes in the database.
This is because QueryBuilder().append(Node, filters={'or': []}, project='id').all() match to every single node and projects its id.

Perhaps this behaviour of QueryBuilder is not as expected? It makes more sense if an exception is raised when the 'or' list contains no element at all. I am not sure if it is still the case in 1.0.0 versions.

Fix a bug such that if no argument is passed to `verdi node delete`. It will try to delete all notes in the database.
This is because `QueryBuilder().append(Node, filters={'or': []}, project='id').all()` match to every single node and projects its id. 
Perhaps this behaviour of `QueryBuilder` is not as expected?
@coveralls
Copy link

coveralls commented Feb 27, 2019

Coverage Status

Coverage decreased (-0.008%) to 56.061% when pulling f1bc36c on zhubonan-patch-1 into f757e55 on release_v0.12.3.

@ltalirz
Copy link
Member

ltalirz commented Feb 28, 2019

@zhubonan thanks for the bug report & the fix!

Perhaps this behaviour of QueryBuilder is not as expected? It makes more sense if an exception is raised when the 'or' list contains no element at all.

As a general rule, I think it makes sense if an "empty" or condition is true.
This can be helpful in nested, autogenerated logic trees where you are passing a list of conditions into "or" that may or may not contain elements.

I can see why it may look a bit confusing, but I think it will also be rare that a user writes {'or': []} by hand.

The bug here is that not providing a node PK results in no filter being applied (rather than doing nothing because of the missing command line arguments).

ltalirz and others added 2 commits February 28, 2019 19:24
Co-Authored-By: zhubonan <33688599+zhubonan@users.noreply.github.com>
@ltalirz ltalirz self-requested a review February 28, 2019 19:29
@zhubonan
Copy link
Contributor Author

I see. Thanks.

@ltalirz
Copy link
Member

ltalirz commented Feb 28, 2019

Just checked that provenance_redesign is not affected.

@ltalirz ltalirz merged commit 29498f2 into release_v0.12.3 Feb 28, 2019
@sphuber sphuber deleted the zhubonan-patch-1 branch March 1, 2019 12:50
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

Successfully merging this pull request may close these issues.

None yet

3 participants