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

[BUG] SecurityManager is deprecated for removal and spotbugs complains about it during build #7079

Open
Azhrei opened this issue Apr 7, 2024 · 3 comments
Assignees
Labels

Comments

@Azhrei
Copy link
Contributor

Azhrei commented Apr 7, 2024

Describe the bug
There are ~36 situations where a SecurityManager is being constructed in the current code base. This class is deprecated and will likely go away very soon (it was deprecated prior to Java 17, then warnings started being issued, and it's likely to go away no later than 24).

To Reproduce
Attempt to run ./gradlew spotbugs and watch the error messages scroll. 🙂

Expected behavior
The build should finish without errors.

Desktop (please complete the following information):

  • OS: Any
  • Version Any

Additional context
All references to the SecurityManager are from the SystemExitInterloper (or is it Intercepter?) class. This class is a kludge to allow a test to call argparse4j.parseArgsOrFail() and for the error to be caught.

However, the latest argparse4j has a method named parseArgs() that throws an IllegalArgumentException instead. This is how the tests should be written.

(I have made proof of concept changes to Main, CommandLine, and CommandLineTest, but I have not pushed them to my repo yet and won't be able to do so for another 24 hours or so. It's at https://github.com/Azhrei/pcgen under the sm-fix2 branch.)

@Azhrei Azhrei added the bug label Apr 7, 2024
@Azhrei
Copy link
Contributor Author

Azhrei commented Apr 8, 2024

I can't seem to assign this to myself (GitHub isn't providing the panel on the right that usually allows labels, assignations, projects, and so forth). But I should have time this coming week to work on it. I'll do a few and try to give an ETA on completion.

@Azhrei
Copy link
Contributor Author

Azhrei commented Apr 13, 2024

Status update: I found two NPEs during slowtest that were the result of my change to the base code. Both will be included in this eventual PR as they're part of this same problem.

@Azhrei
Copy link
Contributor Author

Azhrei commented Apr 22, 2024

Another update. I'm correcting the SecurityManager stuff, but as I touch a given file, I'm trying to cleanup any warnings that IntelliJ is giving me. Some of these source files have a huge number of warnings to go with them.

For example, some of the tests have an assertTrue() or assertFalse() that is testing to ensure a reference is non-null, then the reference is used later in the code — but IntelliJ complains about a possible null because it doesn't know that the assert doesn't return under some circumstances. For those issues, the recommendation in the IDE is to add a Java language assert. That's what I've been doing, on the assumption that the tests aren't production code and we don't care if an extra assert has to run once in awhile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant