Updates cache with response

[NightTripperID]

Updates cache with response

• adds CacheUpdater
• injects CacheUptater into DiscordResponseHandler
• if result does not have both id and guildId fields, cache is not updated
• resolves 111

[NightTripperID]

closing pending further manual testing

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[NightTripperID]

Message models are deserializing with a guildId of 0; the java model contains a guildId field, but the response bodies are arriving without the field. I don't know what the expected behavior is for Message response bodies, but this surprised me.

This implementation currently throws NPE in this scenario. Added check that logs guildId if getCacheForGuild(guildId) == null

[surajkumar]

Thanks for the great contribution! Just check out the comments and this should be good. I don't see any problems with the implementation.

[surajkumar]

Just looking further at the code, perhaps the same should be done to the other methods?

• callAndParseMap
• parseResponseFromList

This would mean iterating over the collections which might tred into some complicated terratory so I'm happy to leave that for a different PR.

On another note: Damn those integration tests being so flakey! (it's discord's fault)

[NightTripperID]

Just looking further at the code, perhaps the same should be done to the other methods?

• callAndParseMap
• parseResponseFromList

This would mean iterating over the collections which might tred into some complicated terratory so I'm happy to leave that for a different PR.

On another note: Damn those integration tests being so flakey! (it's discord's fault)

If I am understanding correctly, you are asking to trace log the cache operation errors at the Map and List levels, which we definitely want.
All of the exception handling and trace logging is taken care of at the element level; no need to add it at the List level. It will also take care of the Map level, because callAndParseMap gets the List specified by the key and caches only that List at the List level.

I did forget to add the caching call in callAndParseMap, so I pushed a commit for that.

I tried running the integration tests from my local and can confirm, Discord was being dumb and just sat there ¯_(ツ)_/¯

[surajkumar]

Just looking further at the code, perhaps the same should be done to the other methods?

• callAndParseMap
• parseResponseFromList

This would mean iterating over the collections which might tred into some complicated terratory so I'm happy to leave that for a different PR.
On another note: Damn those integration tests being so flakey! (it's discord's fault)

If I am understanding correctly, you are asking to trace log the cache operation errors at the Map and List levels, which we definitely want. All of the exception handling and trace logging is taken care of at the element level; no need to add it at the List level. It will also take care of the Map level, because callAndParseMap gets the List specified by the key and caches only that List at the List level.

I did forget to add the caching call in callAndParseMap, so I pushed a commit for that.

I tried running the integration tests from my local and can confirm, Discord was being dumb and just sat there ¯_(ツ)_/¯

Looks good, can we actually comment out the map/list updates? So actually, we need to iterate over the collection and add the item 1-by-1 for example if we receive a List<User> the current code you added will actually cache the entire List object rather than the individual User objects. Reason why this matters is because if a developer wants to look up a specific object e.g. user with id 123456789L, they might not be able to find it because it's stored within a cached List.

I am happy to pick this up a little bit later down the line or you can attempt it yourself should you feel brave :D

The implementation might look something like:

for(T result : resultList) {
cacheUpdater.updateCache(result);
}

Though the real implementation might differ. It should be similar with the Map, just looping over the values and ignoring the keys in that instance.

[surajkumar]

Please see comments, we're almost there!

[NightTripperID]

Just looking further at the code, perhaps the same should be done to the other methods?

• callAndParseMap
• parseResponseFromList

This would mean iterating over the collections which might tred into some complicated terratory so I'm happy to leave that for a different PR.
On another note: Damn those integration tests being so flakey! (it's discord's fault)

If I am understanding correctly, you are asking to trace log the cache operation errors at the Map and List levels, which we definitely want. All of the exception handling and trace logging is taken care of at the element level; no need to add it at the List level. It will also take care of the Map level, because callAndParseMap gets the List specified by the key and caches only that List at the List level.
I did forget to add the caching call in callAndParseMap, so I pushed a commit for that.
I tried running the integration tests from my local and can confirm, Discord was being dumb and just sat there ¯_(ツ)_/¯

Looks good, can we actually comment out the map/list updates? So actually, we need to iterate over the collection and add the item 1-by-1 for example if we receive a List<User> the current code you added will actually cache the entire List object rather than the individual User objects. Reason why this matters is because if a developer wants to look up a specific object e.g. user with id 123456789L, they might not be able to find it because it's stored within a cached List.

I am happy to pick this up a little bit later down the line or you can attempt it yourself should you feel brave :D

The implementation might look something like:

for(T result : resultList) {
cacheUpdater.updateCache(result);
}

Though the real implementation might differ. It should be similar with the Map, just looping over the values and ignoring the keys in that instance.

I think there is a real miscommunication here, we are already adding the items to the cache one element at a time.... the iteration is happening in the cache updater, not the report parser...

public <T> void updateCache(List<T> resultList) {
        resultList.forEach(this::updateCache);
    }

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[surajkumar]

LGTM thanks for the contribution!

Will merge to main tomorrow morning.

[surajkumar]

@NightTripperID accident??