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

Fix XQuery Trigger chaining #4280

Merged
merged 11 commits into from
Mar 31, 2022

Conversation

adamretter
Copy link
Member

@adamretter adamretter commented Mar 22, 2022

Previously it was possible to chain Java Triggers together into pipelines, e.g. the output created by one trigger, can fire events which cause another trigger to take that as input. However, this useful functionality was previously prohibited for XQuery Triggers. It was prohibited, as it was feared that XQuery users could create endless-loops between their Triggers, such behaviour ultimately results in StackOverflowError.

Documentation of behaviour is here - eXist-db/documentation#766.

This PR enables chaining for XQuery Triggers by incorporating an improved cyclic detection mechanism to prevent such StackOverflowError. When a cycle between XQuery Triggers is detected, the cycle is broken and a warning is logged to exist.log. When sensibly planned, users should be able to create XQuery Trigger pipelines without issue. If a mistake is made, they are protected as the cycle will be broken and a warning for them to correct the issue is logged.

Investigating and fixing this also revealed a few small other bugs along the way, for example CreateCollection events were fired during CopyCollection events; This PR corrects those also.

p.s. I have considered in detail the reports from Codacy and Sonarcloud, and I do not agree with their assessment. I think their suggestions would actually make the code harder for others to understand whilst yielding no technical gain.

@adamretter adamretter requested a review from a team March 22, 2022 09:06
@adamretter adamretter added the bug issue confirmed as bug label Mar 22, 2022
@adamretter adamretter added this to the eXist-6.0.2 milestone Mar 22, 2022
@dizzzz
Copy link
Member

dizzzz commented Mar 22, 2022

how 'bad' are the missing triggers? it is not a new issue is it?

@line-o
Copy link
Member

line-o commented Mar 22, 2022

I consider this a feature. I would also like to see the bugs that sonar cloud complains about to be fixed.
At least this must be discussed in detail first.

@line-o line-o self-requested a review March 22, 2022 19:11
line-o
line-o previously requested changes Mar 22, 2022
Copy link
Member

@line-o line-o left a comment

Choose a reason for hiding this comment

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

I would like to see the issues reported in the analysis by sonar and codacy to be addressed.

@line-o line-o added enhancement new features, suggestions, etc. needs documentation Signals issues or PRs that will require an update to the documentation repo and removed bug issue confirmed as bug labels Mar 24, 2022
@adamretter
Copy link
Member Author

adamretter commented Mar 26, 2022

@line-o to try and allay your concerns, I have numbered each item from Codacy in a screenshot, and then explained each individually below.
Codacy

  1. This was previously as laid out in TriggerEvents.java. I preserved it as I believe it might be useful in future. I have now added a log statement to quiet Codacy (although I don't think it is really necessary).

  2. NPathComplexity is not a good metric here. The code itself is simple, straight-forward and understandable by any novice programmer.

  3. Combining the if statements together here would make the intention of the code less clear; the two predicates do not have an implicit relationship!

  4. Same as (2).

  5. Same as (2).

  6. The unused Txn parameter is used in Elemental and FusionDB. It could be used in eXist-db in future if eXist-db chose to properly implement transactions. However, at present it allows a shared API to be used for users triggers across all 3 database systems - i.e. code reuse of EXPath packages. In addition changing it would break existing triggers that are deployed in the wild.

  7. Same as (6).

  8. I have removed the unused parameters apart from Txn (see 6).

  9. Same as (2).

  10. This advice in this context is wrong. Following it would change the behaviour and the code would no longer function correctly.

  11. Same as (2).

  12. This was the case before my changes. I have added a comment to quiet codacy.

  13. Same as (12).

  14. Same as (2).

  15. As opposed to what? If your method is a distinct unit then it is a distinct unit!

  16. This was how the method was named before my changes as this code was taken from the existing XQueryTriggerTest. I have now renamed it in both existing and new places to make Codacy quiet.

  17. This was the case before my changes as this code was as taken from the existing XQueryTriggerTest. I have now adjusted it in both existing and new places to make Codacy quiet.

All-in-all no meaningful issues were discovered by Coadacy. I have appended the commit that quietens Codacy.

@adamretter adamretter removed the needs documentation Signals issues or PRs that will require an update to the documentation repo label Mar 26, 2022
@adamretter
Copy link
Member Author

adamretter commented Mar 26, 2022

@line-o Regards SonarCloud. There are 3 issues suggested, I believe 2 of them are existing issues from how eXist-db's Trigger API is structured. I believe they can safely be ignored in this context. The final issue about calling ThreadLocal.remove() I don't think would have been an issue in this context, however as it is easy to quieten SonarCloud here without changing the behaviour of the code I have addressed it as suggested by SonarCloud with an additional commit.

I hope all your concerns are now addressed.

@sonarcloud
Copy link

sonarcloud bot commented Mar 26, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug B 2 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 50 Code Smells

46.7% 46.7% Coverage
0.0% 0.0% Duplication

@dizzzz dizzzz requested a review from line-o March 31, 2022 11:06
@dizzzz dizzzz dismissed line-o’s stale review March 31, 2022 14:20

all adressed.

@dizzzz dizzzz merged commit fb8b318 into eXist-db:develop Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new features, suggestions, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants