Skip to content

Addressing the system.out lines throughout the project.#121

Merged
wjonassen merged 5 commits intoopendcs:mainfrom
wjonassen:feature/sysout_to_slf4j
Mar 8, 2024
Merged

Addressing the system.out lines throughout the project.#121
wjonassen merged 5 commits intoopendcs:mainfrom
wjonassen:feature/sysout_to_slf4j

Conversation

@wjonassen
Copy link
Collaborator

Problem Description

There are system.out's in the project. We want to either remove them completely (if they are not useful), or replace those with SLF4J in the case that they are useful.

Fixes #79.

Solution

In the cases where it's a system.out that's not useful (maybe it's a debug line), remove it completely. If it is useful, then convert it into slf4j.

how you tested the change

Verify the project builds.

Where the following done:

  • Tests. Check all that apply:
    • Unit tests created or modified that run during ant test.
    • Integration tests created or modified that run during integration testing
      (Formerly called regression tests.)
    • Test procedure descriptions for manual testing
  • Was relevant documentation updated?
  • Were relevant config element (e.g. XML data) updated as appropriate

…one system.out to a throw.

Also, removing unused imports.
Also, renamed mapper in the function to remove confusion with the class variable.
This is a decent amount of them, but getting these changes out now to get some review on them before continuing down the path of doing them all.

Some system.out's are removed, some have been converted to slf4j logging.  Others are converted into throws.
@wjonassen
Copy link
Collaborator Author

@MikeNeilson I have addressed a bunch of the system.out's in this. There are still more, but if you could go through some of the changes to make sure I'm addressing them appropriately, I can then finish off the rest of them.

I am removing the ones where they really shouldn't be there, converting some to slf4j logging, and converting some to thrown exceptions.

Copy link
Collaborator

@MikeNeilson MikeNeilson left a comment

Choose a reason for hiding this comment

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

Looks like a reasonable start to me.

@wjonassen
Copy link
Collaborator Author

Looks like a reasonable start to me.

Ok, sounds good. I will address the rest of the system.out's and 'undraft' this PR.

This is a decent amount of them, but getting these changes out now to get some review on them before continuing down the path of doing them all.

Some system.out's are removed, some have been converted to slf4j logging.  Others are converted into throws.
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 7, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
3.5% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

{
Logger.getLogger(ApiConstants.loggerName).info("PropSpecHelper.getPropSpecs class='" + className + "'");
//className is user controlled, so it is logged at trace level.
LOGGER.trace("PropSpecHelper.getPropSpecs class='{}'", className);

Check notice

Code scanning / SonarCloud

Logging should not be vulnerable to injection attacks

<!--SONAR_ISSUE_KEY:AY4aMkg1JtcO9iJ2BR3Y-->Change this code to not log user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=opendcs_rest_api&issues=AY4aMkg1JtcO9iJ2BR3Y&open=AY4aMkg1JtcO9iJ2BR3Y&pullRequest=121">SonarCloud</a></p>
}

System.out.println(String.format("API Param String: %s", paramString));
LOGGER.debug("API Param String: {}", paramString);

Check notice

Code scanning / SonarCloud

Logging should not be vulnerable to injection attacks

<!--SONAR_ISSUE_KEY:AY4aMkZAJtcO9iJ2BR3C-->Change this code to not log user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=opendcs_rest_api&issues=AY4aMkZAJtcO9iJ2BR3C&open=AY4aMkZAJtcO9iJ2BR3C&pullRequest=121">SonarCloud</a></p>
@wjonassen wjonassen marked this pull request as ready for review March 7, 2024 18:37
@wjonassen
Copy link
Collaborator Author

Looks like a reasonable start to me.

@MikeNeilson I know you did approve this yesterday, but I have just added the rest of the updates for addressing the system.out's and don't want to merge until the new changes are approved (or updates are needed).

@wjonassen wjonassen merged commit 8c31fff into opendcs:main Mar 8, 2024
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.

Replace System.out.println references with SLF4j

2 participants