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

SpotBugs based cleanup in nucleus-admin and surroundings #25149

Merged
merged 39 commits into from
Sep 18, 2024

Conversation

dmatej
Copy link
Contributor

@dmatej dmatej commented Sep 17, 2024

Review

I recommend to review each commit,
Just one note before you notice - the commit "Fixed parsing charset from ContentType..." fixes mistake I made in "SpotBugs-based cleanup in security module" where I tried to parse Charset from the whole content type header, not just the charset parameter.

Changes

  • Mostly explicitly declaring Charse.defaultCharset where using STDIN, STDOUT or STDERR, or respecting contentType header or using UTF-8 as a default (not defaultCharset as we are communicating with other servers with other settings and even local can change between users - or even user can switch his own, encoded files then might stop working).
  • Charset is a feature of a file, not of a user.
  • Refactoring - when I was fixing some incorrect copy pasted code for fifth time, I decided to move it somewhere.
  • When I noticed 1K buffer, I checked how large data it usually processes; mostly I increased the size of the buffer to 8K. Btw Files.readAllBytes uses 8K as a default.
  • Some trivial changes were made automatically by an editor - generics, added finals. Sometimes there could be done more, but I did not want to increase the size of this PR too much. Maybe later with adding Checkstyle rules.
  • Bugs - I noticed several bugs and I fixed them, commits with their fixes are very small, so you will see in the review.

- Reported by SpotBugs as impossible cast
- However it seems this method is unused now.

Signed-off-by: David Matějček <[email protected]>
- Added curly braces
- Removed unused StartupContext field.
- Removed redundant field initialization.

Signed-off-by: David Matějček <[email protected]>
…imeout

- Default is 3 seconds, but tests can be fast enough to make it in 1 second.

Signed-off-by: David Matějček <[email protected]>
- Two "logging" classes with bugs reported by SpotBugs
- Two empty packages

Signed-off-by: David Matějček <[email protected]>
- Created method getCharset in the Utility class with code extracted from two
  convert* methods and then refactored. The charset parameter of those methods
  was then changed from String to Charset, so some callers have to resolve
  the string charset before the call, some used explicit Charset value.
- FileRealmHelper should not depend on default charset, because it can change.
  Using UTF-8 instead.
- TextLoginDialog
  - deleted unused password attribute
  - Logger moved to its only usage
  - Fixed formatting, usage of enhanced loop
- Fixed some javadocs

Signed-off-by: David Matějček <[email protected]>
- Removed printing stacktrace to logs, lets logging do that.
- Use charset from response to parse response, not from own operating system.

Signed-off-by: David Matějček <[email protected]>
Signed-off-by: David Matějček <[email protected]>
- Removed printing stacktrace to logs, lets logging do that.
- Use charset from response to parse response, not from own operating system.

Signed-off-by: David Matějček <[email protected]>
Signed-off-by: David Matějček <[email protected]>
- The class wrote the file with system charset but declared UTF8 in xml
- Constructor called instance methods, now uses just static methods
- The instanceStates property was not used
- Added missing curly braces

Signed-off-by: David Matějček <[email protected]>
- LineTokenReplacer now uses just UTF-8
  - JVM has no power to find out which charset uses the administrator user,
    lets use the standard.
  - Uses try-with and enhanced loop now
- Class attributes moved to the top
- Two String paths changed to File, methods renamed
- Deleted unused TokenReplacementTester

Signed-off-by: David Matějček <[email protected]>
- however parser used default charset and swallowed errors.

Signed-off-by: David Matějček <[email protected]>
- Logging can print stacktraces on its own

Signed-off-by: David Matějček <[email protected]>
- META-INF/hk2-locator/default is encoded in UTF-8
- close() exceptions not swallowed any more
- Removed duplicit isLoggable
- try-with
- getDeclaredConstructor usage
- SimpleInjectionResolver moved to the bottom

Signed-off-by: David Matějček <[email protected]>
Signed-off-by: David Matějček <[email protected]>
- The behavior was correct, the encoding is related to the input file, but
  when reading from console, we always use default charset.

Signed-off-by: David Matějček <[email protected]>
- Uses UTF-8, not default charset
- Fixed unclosed IO warning
- symmetric <ul>  element - if it breaks, you cannot fix it by closing
  the element
- Reordered methods for better readability
- try-with - originally those fields were closed, but were kept after.

Signed-off-by: David Matějček <[email protected]>
…itten

- note: readAllBytes uses 8K buffer which should be sufficient to read all
  used resource files (status.html).

Signed-off-by: David Matějček <[email protected]>
- When printing to stdout with different charset, the byte stream has to be
  reencoded.
- The report has to be encoded in UTF-8
- For XML the application/xml is preferred.
- Optimizations

Signed-off-by: David Matějček <[email protected]>
…ault

- Using Charset.defaultCharset as default can break things when client
  simply did not set the value in the header. Then we cannot assume too much,
  however UTF-8 is an industry standard. In some usage it even does not matter
  as ie. digest numbers can be always encoded to UTF-8 (or even Latin1).
- The original Grizzly's getCharsetFromContentType is not reachable for
  some classloaders and also returned String, so I copy pasted it and changed
  it a bit for the purpose.

Signed-off-by: David Matějček <[email protected]>
- Using UTF-8 instead of default charset
- Throwing exception if method failed, don't hide errors.

Signed-off-by: David Matějček <[email protected]>
- Always use the same UTF-8 charset to encode/decode master password

Signed-off-by: David Matějček <[email protected]>
- Reduced copy-paste
- ContentType is not the only header supporting charsets, Authorization too
- Parsing response from HTTP connection is quite useful
- Resolved hardcoded password - we can generate it for tests
- Resolved dependency on default charset

Signed-off-by: David Matějček <[email protected]>
- Using 8K buffers instead of 1K (default JDK readAllBytes, replaced old impl)
- Using UTF-8 where possible, RestAdapter now specifies charset in contentType
- TextResourcesGenerator now uses Properties to write properties
- Small syntax changes

Signed-off-by: David Matějček <[email protected]>
Signed-off-by: David Matějček <[email protected]>
@dmatej dmatej added bug Something isn't working code cleaning labels Sep 17, 2024
@dmatej dmatej added this to the 7.0.18 milestone Sep 17, 2024
@dmatej dmatej requested review from arjantijms, pzygielo, hs536, avpinchuk and a team September 17, 2024 09:47
@dmatej dmatej self-assigned this Sep 17, 2024
@pzygielo pzygielo removed their request for review September 17, 2024 09:58
@arjantijms
Copy link
Contributor

Just spot checked, but lgtm

@dmatej dmatej merged commit 8fc1afb into eclipse-ee4j:master Sep 18, 2024
2 checks passed
@dmatej dmatej deleted the spotbugs-admin branch September 18, 2024 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working code cleaning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants