-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 concurrent modification exception #4935
Conversation
Your approach of having it in the engine feels like the clean approach to me. Putting it in settings feels like making the settings object cross the boundary implicitly set by its classname. |
core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
Outdated
Show resolved
Hide resolved
The significant amount of additional required That refactoring could be done in a separate and later change. To me it feels a bit weird to first request an analyzer to prepare itself for running on behalf of an engine instance and then requiring the same engine instance to be supplied again on each and every analysis. |
core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
Outdated
Show resolved
Hide resolved
7653303
to
4d1c843
Compare
core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
Outdated
Show resolved
Hide resolved
4d1c843
to
7a52353
Compare
@aikebah thank you for the feedback on the PR. The abstract suppression analyzer now stores a reference to the engine and I also moved the call in the engine to report the unused suppression rules to a new analyzer - so that it is cleaner and not a weird one off solution. |
@jeremylong LGTM apart from the nitpicking review comments of Checkstyle |
7a52353
to
edaba8f
Compare
Supersedes #4923.
@aikebah this is what I was thinking. currently the persistent object store is on the
engine
- if it was moved to the settings object there would likely be fewer code changes needed. Thoughts on this approach?