Javalin: Context::sessionAttribute(String) can return null value if attribute is not set but is typed as 'T' instead of 'T?'

[FredDeschenes]

Actual behavior (the bug)
Getting an attribute with Context::sessionAttribute(String) (which returns null if the attribute isn't set) returns a T instead of T?.

This forces Kotlin users to add @Suppress("SENSELESS_COMPARISON") when checking if an attribute is null (or have an unexpected runtime NullPointerException).

Expected behavior
sessionAttribute(String) should return T? instead of T.

Additional context
Although an easy fix, this will "break" the API for Kotlin users that currently don't have a null check when getting an attribute.

[tipsy]

Thanks @FredDeschenes. This is a bug, so breaking the existing API is not a concern IMO.

[tipsy]

I see that other attribute functions have the same problem:

fun <T> attribute(attribute: String): T
fun <T> attributeMap(): Map<String, T>
fun <T> sessionAttribute(attribute: String): T
fun <T> sessionAttributeMap(): Map<String, T>

If you'd like to create a PR, please include all of them.

[FredDeschenes]

I don't think the ...Map functions should return a Map<String, T?> since they should only return attributes that have been set (and setting an attribute to null appears to remove the attribute in both cases), can you confirm this?

EDIT: Actually, looking into this a bit more, the ...Map functions should return Map<String, Any> as it's highly unlikely that every attribute has the same type. It's also currently possible (although unlikely) for an attribute to be removed between the call to attributeNames and the call to getAttribute. Since it doesn't really make sense to return an attribute's key without the attribute being present, I'll change those return types to Map<String, Any>.

[tipsy]

I don't think the ...Map functions should return a Map since they should only return attributes that have been set (and setting an attribute to null appears to remove the attribute in both cases), can you confirm this? Since it doesn't really make sense to return an attribute's key without the attribute being present, I'll change those return types to Map.

This part I'm not so sure about. This might be stupid design on my part, but it's not a bug. If the key is currently included with a null value, then changing that could break existing code.

I don't think this change should include anything other than making things nullable.

[FredDeschenes]

Yeah I've been looking at all possible paths the setAttribute calls can take in Jetty's source and turns out they don't all have the same side effects when sending a null value: all of them will remove the attribute, except UpgradeHttpServletRequest which will just add the attribute with a null value -_-

I'll go ahead and revert this back to what was already present except for the nullable stuff.