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

Adds per codemod Includes/Excludes #438

Merged
merged 8 commits into from
Aug 23, 2024
Merged

Adds per codemod Includes/Excludes #438

merged 8 commits into from
Aug 23, 2024

Conversation

andrecsilva
Copy link
Contributor

@andrecsilva andrecsilva commented Aug 16, 2024

Codemods now have their own includes/excludes rules and a supports method.

Includes/Excludes rules are specified by PathMatcher patterns that are relative to the repository root.

Before running on a given file, codemods will now perform two checks: includes/excludes and supports.

Codemod specific Includes/excludes check can be overridden by global configuration from the command line (i.e. with --path-include flag).

supports checks the bare-minimum conditions that must be satisfied before the codemod attempts to run. For example,
javaparser codemods will check if the file is a java file and sarif based codemods will check if there is any results for the file.

Refactored some scattered file checks in codemods into the supports method. For example, the VerbTamperingCodemod will now check if the file is a web.xml file with the supports method instead of the visit. This has the benefit of avoiding the generation of empty codetf reports.

@andrecsilva andrecsilva marked this pull request as ready for review August 16, 2024 12:46
@@ -26,6 +26,29 @@ final class WebGoat822Test extends GitRepositoryTest {
super("https://github.com/WebGoat/WebGoat", "main", "e75cfbeb110e3d3a2ca3c8fee2754992d89c419d");
}

private static final String testPathIncludes =
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we expose the actual values so both the tests and the CLI use the same values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CLI doesn't use these default includes anymore.

Do you still want these to be the default values for find-and-fix codemods? Right now every javaparser based one will only have **.java as its pattern.

@@ -29,6 +30,17 @@ protected CompositeJavaParserChanger(final JavaParserChanger... changers) {
this.changers = Arrays.asList(Objects.requireNonNull(changers));
}

@Override
public IncludesExcludesPattern getIncludesExcludesPattern() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem right, but not sure I have another proposal right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally this would be an intersection of all the patterns. But I'm not sure we can calculate this with just the patterns themselves.

@@ -153,6 +163,11 @@ public List<CodeTFReference> getReferences() {
public String getIndividualChangeDescription(Path filePath, CodemodChange change) {
return null;
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

In the most root interface (if you wanted), you could provide a default interface method that returns true.

Copy link
Contributor

Choose a reason for hiding this comment

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

To prevent all these simple implementations.

Copy link
Contributor Author

@andrecsilva andrecsilva Aug 16, 2024

Choose a reason for hiding this comment

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

This was deliberate. Sarif and JavaParser based changers have reasonable defaults (sarif ones will check if the file is in the results and javaparser ones will check if it's a java file).

Notice that all these are RawFileChangers. Not having a default helps to avoid creating codemods that inspects every file by mistake.

RawFileCodemodRunner(
final RawFileChanger changer,
final Path projectDir,
final IncludesExcludes globalIncludesExcludes) {
this.changer = Objects.requireNonNull(changer);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only thing I don't like about this. This kind of subclass checking is a code smell of an architectural problem that will be hard to diagnose and fix later when it causes a problem.

Copy link

sonarcloud bot commented Aug 23, 2024

@andrecsilva andrecsilva merged commit 1d589b3 into main Aug 23, 2024
6 checks passed
@andrecsilva andrecsilva deleted the inex-changes branch August 23, 2024 13:26
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.

2 participants