Add recipe for rule: HttpSessionInvalidate

[evie-lau]

What's changed?

Add recipe to migrate HttpSession:invalidate() (pre-java6) with HttpServletRequest:logout() (java6 and later)

Anything in particular you'd like reviewers to focus on?

I wasn't sure about the location of the test file, and just placed it in the javaee folder. Please let me know if it should be moved anywhere else.

I'm also wondering if I should update the java-ee-7.yml recipeList to include org.openrewrite.java.migrate.javaee6?

Anyone you would like to review specifically?

@cjobinabo
@AnuRam123

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[cjobinabo]

Looks good to me.

[timtebeek]

Hi @evie-lau and welcome to the project! Good to have you onboard to help automate these changes.

I have a bit of a worry about the current implementation of this recipe; perhaps @cjobinabo can help you through a more subtle approach to change this logout behavior, as changing the type will have unintended effects on other parts of the code too.

[timtebeek]

I'm wondering if this might end up changing too broadly; while the current test works, we are in fact changing any reference to HttpSession to a reference to HttpServletRequest, not just when used to call invalidate().

When comparing these docs pages it appears that HttpSession was retained, and perhaps not a direct replacement for HttpServletRequest in all cases
https://docs.oracle.com/javaee/6/api/javax/servlet/http/HttpSession.html
https://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpSession.html
https://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpServletRequest.html

Maybe we need to be more selective, and somehow access the active HttpServletRequest to call logout there.

[evie-lau]

That's a good point. I've been encountering similar requirements for some other recipes I'm looking to create, mainly in needing to filter specific instances.

I didn't notice anything able to do that in the basic java recipes, and was wondering if I would have to create a new recipe to do so.

[timtebeek]

Yes I feel this most likely would have to be a custom recipe, given how you want to instead call a method on an entirely different type instance, that would need to be accessed from somewhere. Perhaps it would be good to start with a more realistic test, and explore the options from there. For instance I know elsewhere we've inserted comments to tell about necessary migration steps that we can't yet automate; that might be the case here if we can't find a request to call logout on.

[evie-lau]

Changed to use a custom recipe.

• When it finds an instance of HttpSession.invalidate(), it looks at the encapsulating MethodDeclaration for the HttpServletRequest parameter, and uses that to call logout()

I don't like this current implementation (specifically the logic for searching the MethodDeclaration for the right parameter), and had trouble finding a cleaner way to do it, so please provide any feedback if it could be improved.

While I think this updated recipe adheres to making changes conservatively to avoid unnecessary changes, I'm not sure if it covers all the cases necessary. But it can always be expanded in the future.

[timtebeek]

Great improvements already; I've added a few more review comments, and our bot did some as well which I've applied.

Understand your hesitancy around finding the method request parameter. Perhaps we can move all of that logic into an appropriately named method with some JavaDoc to document your reservations. That way if we revisit (or bypass) this in the future it's nicely contained.

We can indeed expand the cases covered in the future if needed, by for instance adding a new parameter where applicable (although that may break compilation in rare cases).

[timtebeek]

Thanks a lot! Sorry for the noise with those automated reviews; they're new and was still trying things out for a bit. PR looks great.