Fix: Resolve ListParam equality issue for caching identical requests #2366
Conversation
074c483 to
3c9be01
Compare
ad4133d to
3e44fcb
Compare
|
Hi, I’ve updated the ListParam equality logic to use DeepCollectionEquality for proper caching of identical requests. The changes have passed the stable and beta workflows, but the min workflow is failing due to what seems to be a compatibility issue with dio_web_adapter and the fileAccessMode parameter in Dio.download. Locally, tests which I wrote are passing, and I’ve verified the changes in compatible environments. Could you help me identify if the failure in the min workflow is due to an unsupported configuration or if additional changes are required? |
- Added test cases to validate unequal inputs (different values, different order). - Confirmed current behavior considers order in equality checks.
dio/test/parameter_test.dart
Outdated
| final param3 = const ListParam(['item3', 'item4'], ListFormat.csv); | ||
| final param4 = const ListParam(['item2', 'item1'], ListFormat.csv); | ||
|
|
||
| final cache = {param1: 'Cached Response'}; |
There was a problem hiding this comment.
Maybe we should directly compare their equality rather than using the map?
There was a problem hiding this comment.
Thanks for the suggestion!
I feel keeping the Map test might be useful if someone intends to use ListParam as a key in a Map. However, I’d like to make sure I’m on the right track. Is your suggestion aimed at simplifying the tests, or do you think the Map test is unnecessary in this case? If direct comparison is preferred, I’m happy to follow that direction
There was a problem hiding this comment.
I’ve updated the tests to use direct equality comparison instead of Map key checks. I think it aligns better with the readability and purpose of the tests.
Let me know if there’s anything else to refine!
commit log: 82e2f03
There was a problem hiding this comment.
Is your suggestion aimed at simplifying the tests
Yes exactly. The PR is about the equality of the parameters so we should directly test their equality.
There was a problem hiding this comment.
Thanks! I see your point now, focusing directly on equality makes the purpose of the tests much clearer of the PR. I appreciate the feedback :)
dio/test/parameter_test.dart
Outdated
| import 'package:test/test.dart'; | ||
|
|
||
| void main() { | ||
| test('ListParam equality for Map key', () { |
There was a problem hiding this comment.
Consider making a ListParam test group here.
What this PR does
Resolves #2364
This PR addresses an issue with ListParam equality, where identical requests were not recognized as the same due to reference comparison of lists. This caused multiple identical requests to be sent to the server, even when caching was implemented.
Changes introduced
Updated ListParam's operator == to use DeepCollectionEquality for proper deep equality comparison of list contents.
Added relevant unit tests to validate the fix and ensure no regression.
New Pull Request Checklist
mainbranch to avoid conflicts (via merge from master or rebase)CHANGELOG.mdin the corresponding package