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.

Sign up for GitHub

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

Jump to bottom

[hDJkPkOQ] Add QueryType validation in apoc.trigger.add #3421

Merged
merged 2 commits into from
Jan 26, 2023
Merged

[hDJkPkOQ] Add QueryType validation in apoc.trigger.add #3421

merged 2 commits into from
Jan 26, 2023

Conversation

vga91
Copy link
Collaborator

@vga91 vga91 commented Jan 24, 2023 •
edited
Loading

Added QueryType validations in validateQuery() method,
so as to fail if the procedure tries to execute schema operations.

See point 2 here: neo4j/apoc#208 (review)

Only for 4.4, since the apoc.trigger.add is deprecated and in 5.x the validateQuery() is not longer present.

@vga91 vga91 added 4.4 core-functionality Adding new procedure, function or signature to APOC core labels Jan 24, 2023
@vga91 vga91 mentioned this pull request Jan 24, 2023
Closed
AzuObs
AzuObs suggested changes Jan 25, 2023
View reviewed changes
core/src/test/java/apoc/trigger/TriggerNewProceduresTest.java Outdated
(row) -> fail("Should fail because of unsupported schema operation"));
} catch (RuntimeException e) {
final String expected = "Failed to invoke procedure `apoc.trigger.install`: " +
"Caused by: java.lang.RuntimeException: Supported query types for the operation are [READ_ONLY, WRITE, READ_WRITE]";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anyway that this could be a more user friendly error message?

"The trigger statement must contain READ_ONLY, WRITE, or READ_WRITE query."

@vga91
Copy link
Collaborator Author

vga91 commented Jan 25, 2023 •
edited
Loading

Is there anyway that this could be a more user friendly error message?
"The trigger statement must contain READ_ONLY, WRITE, or READ_WRITE query."

  1. while I was making this change,
    I realized that it would be better to add this thing to the validate too (neo4j/apoc@e5264e0),
    i.e. I added Set.of(Mode.WRITE, Mode.READ, Mode.DEFAULT), and testTriggerInstallWithSchemaProcedure .
    Basically as it was before it didn't give an error with a @Procedure(Mode=SCHEMA) e.g. apoc.schema.assert

  2. Along with that, I've added two parameters to the Util.validateQuery() with custom error messages.

If you think point 1. is unnecessary / to be separated / another card needs to be created, I'll be happy to change the pr :)

@vga91 vga91 force-pushed the validate-trigger-query-operation branch from 81d947f to cbeeb43 Compare January 26, 2023 08:21
AzuObs
AzuObs approved these changes Jan 26, 2023
View reviewed changes
Copy link
Contributor

@AzuObs AzuObs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the change! Looks good 🙂

@vga91 vga91 merged commit ca05324 into 4.4 Jan 26, 2023
@vga91 vga91 deleted the validate-trigger-query-operation branch January 26, 2023 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AzuObs AzuObs AzuObs approved these changes

Assignees

@AzuObs AzuObs

Labels
4.4 core-functionality Adding new procedure, function or signature to APOC core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vga91 @AzuObs