Fix a couple of bugs in klass_for_keypath#56
Fix a couple of bugs in klass_for_keypath#56barendt wants to merge 1 commit intofractaledmind:masterfrom
Conversation
1. If `filter_set_types` is defined by the controller, but the hash it returns doesn't include the keypath, make sure the hash lookup doesn't throw an exception. 2. When inspecting the ActiveRecord columns_hash, make sure an array keypath is converted to a string.
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
|
|
||
| if set.is_a?(ActiveRecord::Relation) || set.view.is_a?(ActiveRecord::Relation) | ||
| klass_type = set.model.columns_hash.fetch(keypath, nil)&.type | ||
| klass_type = set.model.columns_hash.fetch(keypath.join('.'), nil)&.type |
There was a problem hiding this comment.
NilCheck: ActionSet::FilterInstructions#klass_for_keypath performs a nil-check. More info.
| type_declarations = @controller.public_send(:filter_set_types) | ||
| types = type_declarations['types'] || type_declarations[:types] | ||
| klass = types[keypath.join('.')] | ||
| klass = types.fetch(keypath.join('.'), nil) |
There was a problem hiding this comment.
DuplicateMethodCall: ActionSet::FilterInstructions#klass_for_keypath calls 'keypath.join('.')' 2 times. More info.
Codecov Report
@@ Coverage Diff @@
## master #56 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 54 54
Lines 1109 1109
=====================================
Hits 1109 1109
Continue to review full report at Codecov.
|
|
@barendt: This fix actually helps to reveal a bug in that method. This bit of code currently doesn't work: if set.is_a?(ActiveRecord::Relation) || set.view.is_a?(ActiveRecord::Relation)
klass_type = set.model.columns_hash.fetch(keypath.join('.'), nil)&.type
return klass_type.class if klass_type
endThis is because Also, when fixing bugs (which I greatly appreciate), I would quadruple appreciate if you could add just one regression test for the bugs. The test suite is the backbone of this entire project. |
|
Note: I am working on fixing the issue I describe above |
|
Efficiently typecasting filtering params across all of the various scenarios this gem handles (now that I am thinking clearly and exhaustively about this problem) is a complex problem. I am on the right track, but I won't be able to finish things up tonight. I should be able to get things working over the weekend tho. I will tag you both in the PR that arises so that you can keep tabs on the progress and solution. Sorry that once again your (much appreciated) work to resolve bugs that you found wasn't quite enough to appease the gluttonous and omnivorous test suite. And I hope it isn't frustrating to have me take over your initial work. However, both of these times, you have presented great ideas or important bugs that have required more substantive work to build in fully robust ways. But you providing such clear starting points has truly been invaluable to me, so I thank you. Hopefully we can both have what we want (you, a gem that works well and efficiently for your specific apps; me, a gem that works for every logical situation I can imagine) with regard to filtering requests by the end of this weekend. |
|
That all makes sense to me. Sorry about submitting a PR with no test cases. For this PR, where would you like to see tests? The test suite here is pretty DRY/meta/requesty, so I'm not sure where best to add these. My first instinct is to want to create a |
|
Yes. Some directory like |
2 participants
This fixes a couple of bugs I hit in the new type-hinting code:
If
filter_set_typesis defined by the controller, but the hash it returns doesn't include the keypath, make sure the hash lookup doesn't throw an exception.When inspecting the ActiveRecord columns_hash, make sure an array keypath is converted to a string.