Make the FieldQueryMapEncoder encoder thread safe #1369
Conversation
Budlee
force-pushed
the
FieldEncoderThreadSafe
branch
from
e56be5f
to
3cca348
Compare
| @@ -56,7 +61,7 @@ private ObjectParamMetadata getMetadata(Class<?> objectType) { | |||
| ObjectParamMetadata metadata = classToMetadata.get(objectType); | |||
| if (metadata == null) { | |||
| metadata = ObjectParamMetadata.parseObjectType(objectType); | |||
| classToMetadata.put(objectType, metadata); | |||
| classToMetadata.putIfAbsent(objectType, metadata); | |||
There was a problem hiding this comment.
Could this whole method be replaced with:
return classToMetadata.computeIfAbsent(objectType, ObjectParamMetadata::parseObjectType);
???
There was a problem hiding this comment.
I think it could,
I wasn’t sure on the refactor culture. Some like to make as few changes as possible.
happy to add this in
There was a problem hiding this comment.
Updated,
the other change I thought was to the core of this loop to be the following, please let me know if its okay as I woulduse a pair:
return metadata.objectFields.stream()
.map(this::pairFieldAndOptionalValue)
.filter(pair -> pair.right.isPresent())
.collect(Collectors.toMap(pair -> createKeyFromField(pair.left), pair -> pair.right));Its much smaller (personally I think it reads better), however, I understand it is not to everyones taste.
Some do not like the use of pair and also pairFieldAndOptionalValue would have a hidden side effect where if the field.get(object); fails will throw then EncodeException.
There was a problem hiding this comment.
@rage-shadowman what are thoughts on this refactor?
There was a problem hiding this comment.
I think it all depends on how field.get fails. The current loop is vulnerable to that now. I don't have any issue with catching there and wrapping in an EncodeException, that fits with the Encoder interface, so it's not a hidden side-effect, so long as you add some comments around the check.
There was a problem hiding this comment.
If you're asking my opinion, I think that any time you simplify the code without breaking anything, you make it better. I think this PR looks more simplified.
As far as that stream usage goes, it looks more difficult to read at-a-glance to me, but the original isn't that easy to read at-a-glance either, and I tend to be biased against all but the simplest of streams usage.
As long as your stream doesn't modify anything from the input metadata, or from one of the earlier map()d records, then there aren't any side effects. Throwing a runtime exception due to an unexpected illegal state sounds reasonable.
Change the caching map to be concurrent and only insert items that are missing to the map. Fixes OpenFeign#1257
Budlee
force-pushed
the
FieldEncoderThreadSafe
branch
from
3cca348
to
002e660
Compare
|
@rage-shadowman and @kdavisk6 I have updated this now, thanks |
|
@Budlee Done is better than perfect here. |
kdavisk6
added
ready to merge
|
@kdavisk6 , when does this get merged? |
Change the caching map to be concurrent and only
insert items that are missing to the map.
Fixes #1257