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

CodeQL fixups #2134

Closed
wants to merge 21 commits into from
Closed

CodeQL fixups #2134

wants to merge 21 commits into from

Conversation

jas88
Copy link
Contributor

@jas88 jas88 commented Feb 13, 2025

Proposed Change

Fix some CodeQL warnings and one error flagged after #2133

Type of change

What types of changes does your code introduce? Tick all that apply.

  • Bugfix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation-Only Update
  • Other (if none of the other choices apply)

Checklist

By opening this PR, I confirm that I have:

  • Ensured that the PR branch is in sync with the target branch (i.e. it is automatically merge-able)
  • Created or updated any tests if relevant
  • Have validated this change against the Test Plan
  • Requested a review by one of the repository maintainers
  • Have written new documentation or updated existing documentation to detail any new or updated functionality and how to use it
  • Have added an entry into the changelog

jas88 added 2 commits February 13, 2025 10:03
@jas88
CodeQL fixups
a9f6abb
@jas88
Update JoinInfo.cs
c0f1414 
Finally fix that "Joink" typo after all the others
@jas88 jas88 requested a review from JFriel February 13, 2025 19:52
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 13, 2025
View reviewed changes
Rdmp.UI.Tests/DesignPatternTests/ClassFileEvaluation/DocumentationCrossExaminationTest.cs
Comment on lines +500 to +538
foreach (var match in rWords.Matches(line).Where(match => glossaryHeaders.Contains(match.Value)))
{
//It's already got a link on it e.g. [DBMS] or it's "UNION - sometext"
if (match.Index - 1 > 0
&&
(line[match.Index - 1] == '[' || line[match.Index - 1] == '"'))
continue;


var path1 = new Uri(mdFile);
var path2 = new Uri(glossaryPath);
var diff = path1.MakeRelativeUri(path2);
var relPath = diff.OriginalString;

if (!relPath.StartsWith('.'))
relPath = $"./{relPath}";

var suggestedLine = $"[{match.Value}]: {relPath}#{match.Value}";
var markdownLink = $"[{match.Value}]({relPath}#{match.Value})";

//if it has spaces on either side
if (line[Math.Max(0, match.Index - 1)] == ' ' && line[
Math.Min(line.Length - 1,
match.Index + match.Length)] == ' '
//don't mess with lines that contain an image
&& !line.Contains("!["))
allLines[lineNumber - 1] = line.Replace($" {match.Value} ", $" [{match.Value}] ");

//also if we have a name like `Catalogue` it should probably be [Catalogue] instead so it works as a link
allLines[lineNumber - 1] = line.Replace($"`{match.Value}`", $"[{match.Value}]");

//if it is a novel occurrence
if (!allLines.Contains(suggestedLine) && !suggestedLinks.ContainsValue(suggestedLine) && !allLines.Contains(markdownLink) && !suggestedLinks.ContainsValue(markdownLink))
{
suggestedLinks.Add(match.Value, suggestedLine);
problems.Add(
$"Glossary term should be link in {mdFile} line number {lineNumber}. Term is {match.Value}. Suggested link line is:\"{suggestedLine}\"");
}
}

Check notice

Code scanning / CodeQL

Missed opportunity to use Where Note test

This foreach loop
implicitly filters its target sequence
Loading
- consider filtering the sequence explicitly using '.Where(...)'.
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 14, 2025
View reviewed changes
Rdmp.UI/AggregationUIs/AggregateGraphUI.cs


var startDate = ConvertAxisOverridetoDate(startDateOverride.Trim('\''), incriment);
var endDate = ConvertAxisOverridetoDate(endDateOverride.Trim('\''), incriment);
var startDate = ConvertAxisOverridetoDate(startDateOverride.Trim('\''), increment);

Check warning

Code scanning / CodeQL

Dereferenced variable may be null Warning

Variable
startDateOverride
Loading
may be null at this access as suggested by
this
Loading
null check.
Variable
startDateOverride
Loading
may be null at this access because the parameter has a null default value.

Copilot Autofix AI 18 days ago

To fix the problem, we need to ensure that startDateOverride is not null before it is dereferenced on line 305. The best way to fix this is to add a null check for startDateOverride before it is used. If startDateOverride is null, we can either set a default value or handle the situation appropriately, such as by returning early from the method.

Suggested changeset 1
Rdmp.UI/AggregationUIs/AggregateGraphUI.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/Rdmp.UI/AggregationUIs/AggregateGraphUI.cs b/Rdmp.UI/AggregationUIs/AggregateGraphUI.cs
--- a/Rdmp.UI/AggregationUIs/AggregateGraphUI.cs
+++ b/Rdmp.UI/AggregationUIs/AggregateGraphUI.cs
@@ -303,2 +303,4 @@
 
+                if (startDateOverride == null || endDateOverride == null)
+                    throw new ArgumentNullException("startDateOverride or endDateOverride cannot be null during refresh.");
 
EOF
@@ -303,2 +303,4 @@

if (startDateOverride == null || endDateOverride == null)
throw new ArgumentNullException("startDateOverride or endDateOverride cannot be null during refresh.");

Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Rdmp.UI/AggregationUIs/AggregateGraphUI.cs
var startDate = ConvertAxisOverridetoDate(startDateOverride.Trim('\''), incriment);
var endDate = ConvertAxisOverridetoDate(endDateOverride.Trim('\''), incriment);
var startDate = ConvertAxisOverridetoDate(startDateOverride.Trim('\''), increment);
var endDate = ConvertAxisOverridetoDate(endDateOverride.Trim('\''), increment);

Check warning

Code scanning / CodeQL

Dereferenced variable may be null Warning

Variable
endDateOverride
Loading
may be null at this access as suggested by
this
Loading
null check.
Variable
endDateOverride
Loading
may be null at this access because the parameter has a null default value.

Copilot Autofix AI 18 days ago

To fix the problem, we need to ensure that endDateOverride is not null before it is dereferenced. The best way to fix this without changing existing functionality is to add a null check for endDateOverride before it is used on line 306. If endDateOverride is null, we should handle it appropriately, possibly by skipping the relevant code block or providing a default value.

Suggested changeset 1
Rdmp.UI/AggregationUIs/AggregateGraphUI.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/Rdmp.UI/AggregationUIs/AggregateGraphUI.cs b/Rdmp.UI/AggregationUIs/AggregateGraphUI.cs
--- a/Rdmp.UI/AggregationUIs/AggregateGraphUI.cs
+++ b/Rdmp.UI/AggregationUIs/AggregateGraphUI.cs
@@ -305,3 +305,3 @@
                 var startDate = ConvertAxisOverridetoDate(startDateOverride.Trim('\''), increment);
-                var endDate = ConvertAxisOverridetoDate(endDateOverride.Trim('\''), increment);
+                var endDate = endDateOverride != null ? ConvertAxisOverridetoDate(endDateOverride.Trim('\''), increment) : DateTime.MaxValue;
                 var rowsToDelete = _dt.Rows.Cast<DataRow>().Where(r =>
EOF
@@ -305,3 +305,3 @@
var startDate = ConvertAxisOverridetoDate(startDateOverride.Trim('\''), increment);
var endDate = ConvertAxisOverridetoDate(endDateOverride.Trim('\''), increment);
var endDate = endDateOverride != null ? ConvertAxisOverridetoDate(endDateOverride.Trim('\''), increment) : DateTime.MaxValue;
var rowsToDelete = _dt.Rows.Cast<DataRow>().Where(r =>
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
jas88 added 2 commits February 13, 2025 18:54
@jas88
Redundant control flow statements
781807a
@jas88
Update Validation.md
10b9e42 
Update docs for PluginSecondaryConstraint replacement with SecondaryConstraint
@jas88 jas88 marked this pull request as draft February 14, 2025 01:48
@jas88 jas88 closed this Feb 14, 2025
@jas88 jas88 deleted the feature/codeql-fixes branch February 14, 2025 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JFriel JFriel Awaiting requested review from JFriel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@jas88