fix: Allow to remove a ThreadLocalAccessor by Key of Type Object

[boubaker]

The ThreadLocalAccessor.key() is of Type Object. Thus, when it comes to delete a predefined ThreadLocalAccessor which have a non-String key, it's becomes impossible to delete. This behavior was observed while attempting to remove org.springframework.security.core.context.ReactiveSecurityContextHolderThreadLocalAccessor from the ContextRegistry, which have a key of type java.lang.Class.

This change will just switch the method parameter in order to support the Type java.lang.Object as key, which shouldn't be a breaking change, even in a minor version.

[shakuzen]

This change will just switch the method parameter in order to support the Type java.lang.Object as key, which shouldn't be a breaking change, even in a minor version.

I'd guess it's a breaking change for binary compatibility (side note: we should probably have japicmp setup to detect breaking changes). I think we can work around it by having a method that takes a String and delegates to the new method with argument type Object.

[chemicL]

This change will just switch the method parameter in order to support the Type java.lang.Object as key, which shouldn't be a breaking change, even in a minor version.

I'd guess it's a breaking change for binary compatibility (side note: we should probably have japicmp setup to detect breaking changes). I think we can work around it by having a method that takes a String and delegates to the new method with argument type Object.

I agree :)

However, I fear we should not have a situation like this:

    public boolean removeThreadLocalAccessor(String key) {
        return removeThreadLocalAccessor((Object) key);
    }

    public boolean removeThreadLocalAccessor(Object key) {
        for (ThreadLocalAccessor<?> existing : this.threadLocalAccessors) {
            if (existing.key().equals(key)) {
                return this.threadLocalAccessors.remove(existing);
            }
        }
        return false;
    }

I am worried about this for two reasons:

• Because this is confusing, despite the argument being named "key". Perhaps in the future we'd like to offer the means to remove the actual accessor.
• Perhaps we should deprecate the existing method to keep the API consistent. If we do that, the users would have to do the casting in their codebases to avoid deprecation warnings, like removeThreadLocalAccessor((Object) myStringKey).

How about the existing method gets deprecated and a new method is added:

    @Deprecated
    public boolean removeThreadLocalAccessor(String key) {
        return removeThreadLocalAccessorForKey(key);
    }

    public boolean removeThreadLocalAccessorForKey(Object key) {
        for (ThreadLocalAccessor<?> existing : this.threadLocalAccessors) {
            if (existing.key().equals(key)) {
                return this.threadLocalAccessors.remove(existing);
            }
        }
        return false;
    }

[boubaker]

How about the existing method gets deprecated and a new method is added

Sure, feel free to change the PR or to close it by adding a new one.
For now, I have made a workaround using a simple Java Reflection access.

Thanks.

[rstoyanchev]

Deprecation in a minor or major sounds fine to eventually migrate to a corrected method signature. For a backport, we could simply introduce the additional method to avoid any potential disruption.

[chemicL]

@boubaker are you interested in modifying this PR according to the above consensus?

• target this PR against the 1.1.x branch and add the new removeThreadLocalAccessorForKey(Object key) method
• open a new PR against main with the deprecation for the existing method

[boubaker]

@boubaker are you interested in modifying this PR according to the above consensus?

• target this PR against the 1.1.x branch and add the new removeThreadLocalAccessorForKey(Object key) method
• open a new PR against main with the deprecation for the existing method

Yes, I 'm ok with the fix as well. Thanks for your help
It could be simpler if you create the PRs (Actually, the PR build fails due to a formatting error)

[boubaker]

Please let me know if that's ok for you, I 'll close this one.