Issue-637 #1802

ramraval · 2021-11-11T01:13:16Z

Addresses Issue #637 by adding new detector & associated test case classes for bad combinations of format strings used with SimpleDateFormat

(Apologies for any confusion, I know @arnab44 had been assigned to this issue about a month ago, but I am a first-time contributor who took up this issue for a university project)

Make sure these boxes are checked before submitting your PR -- thank you!

Added an entry into CHANGELOG.md if you have changed SpotBugs code

…at String Checks"

-Split "Issue637" class into two separate classes, one expecting errors and one expecting no errors -Refactored "Issue637Test" file to reflect new, simplified test format -Added JavaDocs for public functions

…d constructors to DateFormatStringChecker -Added tests for new functionality to corresponding test classes

ThrawnCA · 2021-11-11T23:04:17Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/DateFormatStringChecker.java

@@ -0,0 +1,137 @@
+/*
+ * FindBugs - Find bugs in Java programs


New files should not use a FindBugs copyright notice.

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/DateFormatStringChecker.java

KengoTODA

Thanks for your contribution!
Please check my feedback to keep the code maintainable. Also, run ./gradlew spotlessApply to format java code.

spotbugs/etc/findbugs.xml

spotbugs/etc/messages.xml

spotbugs/etc/findbugs.xml

KengoTODA · 2021-11-15T23:11:20Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/DateFormatStringChecker.java

+                AM_PM_HOURS_FLAG_1, AM_PM_HOURS_FLAG_2, MILITARY_HOURS_FLAG_1, MILITARY_HOURS_FLAG_2, WEEK_YEAR_FLAG_1
+        ));
+
+        if (CONST_ARRAY_LIST.contains(seen) && stack.getStackDepth() > 0) {


better to apply the early return, to keep nests in the code less.

if (!CONST_ARRAY_LIST.contains(seen) || stack.getStackDepth() == 0) { return; }

KengoTODA · 2021-11-15T23:12:10Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/DateFormatStringChecker.java

+
+    private static final String BUG_TYPE = "FS_BAD_DATE_FORMAT_FLAG_COMBO";
+
+    String dateFormatString;


This field is used only by one method, then better to replace it with a local variable.

KengoTODA · 2021-11-15T23:13:25Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/DateFormatStringChecker.java

+        return result;
+    }
+
+    private boolean checkStringForBadCombo(String stringToCheck, String missingFlag, List<String> requiredFlags) {


For me runCheckStringForBadCombo and checkStringForBadCombo sounds the same.
Could you consider renaming them, or adding documentation comments to clarify the difference?

KengoTODA · 2021-11-15T23:23:24Z

spotbugs/etc/messages.xml

+This format stream includes a bad combination of format flags which may lead to unexpected behavior. Examples include
+using a week year ("Y") without specifying week in year ("w") or using an AM/PM hour ("h" or "K") without specifying
+an AM/PM marker ("a").


Consider providing the list of full examples.
Users will not read SpotBugs source code, we need a document or URL to tell 'what is the problem' and 'how you can fix it'.

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/DateFormatStringChecker.java

KengoTODA · 2021-11-15T23:32:05Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/DateFormatStringChecker.java

+    private static final List<String> AM_PM_HOURS_FLAG_2 = new ArrayList<>(Arrays.asList("a", "K"));
+    private static final List<String> MILITARY_HOURS_FLAG_1 = new ArrayList<>(Arrays.asList(null,"H","a"));
+    private static final List<String> MILITARY_HOURS_FLAG_2 = new ArrayList<>(Arrays.asList(null,"k","a"));
+    private static final List<String> WEEK_YEAR_FLAG_1 = new ArrayList<>(Arrays.asList("w","Y","M","d"));


In this list Arrays.asList("w","Y","M","d") the first element has different meaning. It would be better to introduce a class to replace this list.

private static final class DateFormatRule { /** Required flag to run verification by this rule. Always apply rule if this field is {@code null}. */ @Nullable String precondition; /** List of required flags in the pattern. */ @NonNull List<String> requiredFlags; /** @return {@code true} if precondition matches and all required flags exist in the given pattern. */ boolean verify(String dateFormatString) { ... } }

Co-authored-by: Kengo TODA <skypencil+github@gmail.com>

-Updates description for FS_BAD_DATE_FORMAT_FLAG_COMBO in "messages.xml" -Reflects suggested changes to DateFormatStringChecker, including adding a nested DateFormatRule object and other smaller updates

ramraval · 2021-11-18T02:08:49Z

Thank you for the feedback, @KengoTODA! I have reflected your suggestions in the latest commit. Please let me know what you think.

JuditKnoll · 2023-07-27T14:51:07Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/DateFormatStringChecker.java

+    private static final String BUG_TYPE = "FS_BAD_DATE_FORMAT_FLAG_COMBO";
+
+    String dateFormatString;
+    final BugReporter bugReporter;


This field could be private.

JuditKnoll · 2023-07-27T14:56:02Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/DateFormatStringChecker.java

+            if (this.dateFormatString != null && (sig.indexOf(SIG_CONSTANT_OPERAND_1) >= 0 ||
+                    sig.indexOf(SIG_CONSTANT_OPERAND_2) >= 0 || sig.indexOf(SIG_CONSTANT_OPERAND_3) >= 0 )
+                    && CLASS_CONSTANT_OPERAND.equals(cl) && (NAME_CONSTANT_OPERAND_1.equals(nm)
+                    || NAME_CONSTANT_OPERAND_2.equals(nm) || NAME_CONSTANT_OPERAND_3.equals(nm))) {


To enhance readability, it's better to have the related parts in one line, and have the line break consistently before the || and && signs.

JuditKnoll · 2023-07-27T15:01:13Z

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue637Test.java

+        BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType(DESIRED_BUG_TYPE)
+                .build();
+        assertThat(getBugCollection(), containsExactly(25, bugTypeMatcher));


Could you please add asserts for every bug checking the information you added to the BugInstance (e.g. containing method, source line)?

ramraval added 4 commits November 9, 2021 16:47

Draft of code implemented to address GitHub issue 637 "SimpleDateForm…

43f5ae6

…at String Checks"

-Refactored "DateFormatStringChecker" class

4729958

-Split "Issue637" class into two separate classes, one expecting errors and one expecting no errors -Refactored "Issue637Test" file to reflect new, simplified test format -Added JavaDocs for public functions

-Added functionality to handle additional SimpleDateFormat methods an…

91a3dad

…d constructors to DateFormatStringChecker -Added tests for new functionality to corresponding test classes

-Added entry to CHANGELOG.md

b1513f3

ThrawnCA requested changes Nov 11, 2021

View reviewed changes

-Addressed requested changes in PR spotbugs#1802

3b86be5

KengoTODA requested changes Nov 15, 2021

View reviewed changes

ramraval and others added 3 commits November 16, 2021 11:43

Update spotbugs/etc/messages.xml

df71ed8

Co-authored-by: Kengo TODA <skypencil+github@gmail.com>

Apply suggestions from code review

62c6366

Co-authored-by: Kengo TODA <skypencil+github@gmail.com>

Apply suggestions from code review (DateFormatStringChecker)

d9518d5

Co-authored-by: Kengo TODA <skypencil+github@gmail.com>

ThrawnCA previously approved these changes Nov 17, 2021

View reviewed changes

Applies additional suggestions from code review

212a1fb

-Updates description for FS_BAD_DATE_FORMAT_FLAG_COMBO in "messages.xml" -Reflects suggested changes to DateFormatStringChecker, including adding a nested DateFormatRule object and other smaller updates

ramraval dismissed ThrawnCA’s stale review via 212a1fb November 18, 2021 01:57

ThrawnCA approved these changes Nov 19, 2021

View reviewed changes

JuditKnoll requested changes Jul 27, 2023

View reviewed changes

github-actions bot added the Stale label Mar 4, 2024

github-actions bot closed this Apr 8, 2024

hazendaz reopened this Apr 12, 2024

github-actions bot removed the Stale label Apr 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue-637 #1802

Issue-637 #1802

ramraval commented Nov 11, 2021

ThrawnCA Nov 11, 2021

KengoTODA left a comment

KengoTODA Nov 15, 2021

KengoTODA Nov 15, 2021

KengoTODA Nov 15, 2021

KengoTODA Nov 15, 2021

KengoTODA Nov 15, 2021 •

edited

ramraval commented Nov 18, 2021

JuditKnoll Jul 27, 2023

JuditKnoll Jul 27, 2023

JuditKnoll Jul 27, 2023


		private static final String BUG_TYPE = "FS_BAD_DATE_FORMAT_FLAG_COMBO";

		String dateFormatString;

Issue-637 #1802

Are you sure you want to change the base?

Issue-637 #1802

Conversation

ramraval commented Nov 11, 2021

Choose a reason for hiding this comment

KengoTODA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KengoTODA Nov 15, 2021 • edited

Choose a reason for hiding this comment

ramraval commented Nov 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KengoTODA Nov 15, 2021 •

edited