remove dependency on servlet API (use case where not needed) #772
Conversation
There was a problem hiding this comment.
@JosephWitthuhnTR - This looks okay to me, although I wouldn't get your hopes up too much. You are likely to run into other s/javax.servlet/jakarta.servlet/ issues at some point.
@jeremiahjstacey - I'm just looking at this. It LGTM, but the changes are the code that you wrote, so I'm going to let you approve this PR (or not).
|
I have no concerns with the code or tests as written. I think the value of this update is more related to the intended content of the next ESAPI release. @kwwall -- do we plan on addressing the javax/jakarta package updates in any other way for the next ESAPI release? If we do, then this change does not matter. The order of the logic isn't going to make a difference if we plan to account for it another way. It is change for the sake of change, which I tend to question. OTOH if we want to create a release with this content before any other updates are made then it will provide the value that @JosephWitthuhnTR has identified. Either way the code can be merged, depending on the release plan I'm not sure it should be due to the possible overlap with the intended value. @JosephWitthuhnTR - I do not intend to discount your contribution. I appreciate the fact that you're calling out in the PR description this is not a long-term fix for the problem, as well as the fact that you are invested enough to make time to offer the changes back to the project. Thank you for your feedback and contributions to the project. |
|
@kwwall We've tested our application with this after making this modification to our JAR, and we don't run into any other issues. Again, this is for the limited use case where we are just using the logging part of the ESAPI library and we turn the IP logging off. So I am reasonably sure that this fixes our use case (and also pretty certain that it doesn't address the larger problem). What I can't speak to is how many other users (if any) use the JAR for as limited of a use case as we do. This could either fix it for nobody else, or it could help a bunch of people. I did notice that the other individual who submitted the bug (issue #767 and discussion #768) (@guadgarcia) was running into the exact same stack trace / use case as our team was, so it seems that it may help them too. @jeremiahjstacey I 100% agree. If a new release with the larger fix is "imminent" or "soon", then there is no reason for this modification (and no reason to merge this PR). If that is still weeks/months off, this change "lets us get by" with a smaller fix in the meantime while we wait for the larger fix. |
There was a problem hiding this comment.
If @jeremiahjstacey is okay with this, then I will approve it. No promises of how soon we get it into a new release though and this barely scratches the surface of the underlying issue.
|
@JosephWitthuhnTR - While I approve this, it would be nice if you could sign your commit with your GPG key, If that is too much trouble, I can bypass that, but Ive added it as part of the branch protection so ideally you can sign a commit that would be better. |
|
@kwwall I've made a mess of this trying to get this right. I'll make a new PR. |
This change deals with issue #767.
It does not attempt to resolve the overall issue with Servlet API version compatibility. What it does is remove the dependency on Servlet API for the use case where a user is trying to use the logger functionality, but has "logServerIP" set to false. When not logging the server IP, there is no reason for this code to depend on Servlet API at all.
This allows us to avoid runtime exceptions if we use this code with the new Jakarta package version of Servlet API, in cases where logServerIP is disabled.
Again, this is not a fix for the overall problem. This is a small change that will let some limited use cases not fail due to the problem (so that those use cases can get by until this problem is solved in a more holistic way).
The change stems from part of this discussion:
#768 (comment)