Relation dynamic finders

[ipundit]

Implemented dynamic finders for Relation:

Author::select('author_id')->find_by_name('Tito');

This greatly reduces the need for where clauses, while avoiding the performance hit of retrieving all the columns for a row as in:

Author::find_by_name('Tito');

[codecov]

Codecov Report

Merging #81 (06b40bb) into master (6bd6d72) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master      #81      +/-   ##
============================================
+ Coverage     96.59%   96.73%   +0.13%     
- Complexity      962      964       +2     
============================================
  Files            33       33              
  Lines          2351     2358       +7     
============================================
+ Hits           2271     2281      +10     
+ Misses           80       77       -3     

Files Changed	Coverage Δ
lib/Model.php	98.62% <100.00%> (+0.58%)	⬆️
lib/Relation.php	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[shmax]

I'll have to dig into this more deeply later, but are you sure we should support this? Modern versions of Rails seem to encourage the use of where over dynamic finders and I can't seem to find any samples of this kind of thing in the modern documentation. Were you able to find anything?

[ipundit]

No, but this is quite a useful feature that I would use everywhere in my production code when moving to version 2. The new find() will return an array or just a model or null depending on the input, thus a lot of my code thus breaks in version 2, but you made the choice to do that and the many users of version 1 will have to update their code to match.

If you always have to check for an array or a single model throughout your code, it gets tedious and error prone. The workaround I came up with, and I would suggest putting in the documentation is to move to find_by_syntax() which will always return just a Model or null. But, the downside of moving to dynamic finders is the loss of select() for performance purposes, which is now fixed.

Also, dynamic finders prevent sql injection attacks whereas where() does not in all cases. I don't see any indication in the documentation where dynamic finders are being deprecated, nor would I expect there to be as it's a readable convenience for Ruby on Rails and ActiveRecord.

[shmax]

No, but this is quite a useful feature that I would use everywhere in my production code when moving to version 2. The new find() will return an array or just a model or null depending on the input, thus a lot of my code thus breaks in version 2, but you made the choice to do that and the many users of version 1 will have to update their code to match.

Wait, hang on--the new find works pretty much like the old one, doesn't it? It returns Model|array<Model>. It only returned null if you passed in 'first' or 'null', so yes, you would have to refactor those usages. Is that what you mean?

If you always have to check for an array or a single model throughout your code, it gets tedious and error prone.

That's not the case, and never was. You don't have to check if the return type is single or array; you have prior knowledge of what is going to come out the other end based on what you pass in:

// always single model (or exception):
$book = Book::find(1);

// always array (or exception)
$books = Book::find([1,2,3]):

// always single model or null
$book = Book::first()

The workaround I came up with, and I would suggest putting in the documentation is to move to find_by_syntax() which will always return just a Model or null. But, the downside of moving to dynamic finders is the loss of select() for performance purposes, which is now fixed.

I'm not sure what you mean. As far as I can tell, all of the find_by_ methods should return a single record, or null.

I don't see any indication in the documentation where dynamic finders are being deprecated, nor would I expect there to be as it's a readable convenience for Ruby on Rails and ActiveRecord.

Okay, then sounds like we can go with it.

[shmax]

Please make sure to implement the proper static checking extensions. You can basically clone ModelStaticMethodReflection (should be able to DRY using a trait like I did for ModelDynamicStaticMethodReturnTypeReflection and RelationDynamicMethodReturnTypeReflection ). You'll need to add a few lines to phpstan.neon.dist. Once you have that, please add a new file (eg. "RelationDynamicFindBy" modified from "ModelDynamicFindBy") to test/phpstan, and make sure that composer stan passes.

edit: don't forget that we have that weird ActiveRecordPHPStanTest file for debugging extensions, but huge bonus points if you can figure out how to debug PHPStan without it (and teach me).

[ipundit]

Please make sure to implement the proper static checking extensions. You can basically clone ModelStaticMethodReflection (should be able to DRY using a trait like I did for ModelDynamicStaticMethodReturnTypeReflection and RelationDynamicMethodReturnTypeReflection ). You'll need to add a few lines to phpstan.neon.dist. Once you have that, please add a new file (eg. "RelationDynamicFindBy" modified from "ModelDynamicFindBy") to test/phpstan, and make sure that composer stan passes.
I do not know how to do this, nor do I even know what is the expected outcome of implementing the above. How does that change the code in Relation.php? I think the easiest way to resolve this is for you to do it this time and show me the diffs so I know what to do next time.

[shmax]

Did you mean to close this?

[shmax]

I do not know how to do this, nor do I even know what is the expected outcome of implementing the above.

It's all about the static checking. We already had this set up for Model, such that PHPStan would know to flag this kind of thing as an error:

$book = Book::find_by_name("Walden");
if(is_array($book)) {  // static analysis error! $book can't be an array, so there is no point in testing it as one

This kind of magic is currently achieved through the PHPStan reflection class I authored a few weeks back, ModelStaticMethodReflection.

Now that you've migrated to Relation, we have to make sure PHPStan knows about it. Since the only difference between the find_by_ calls on Relation and the ones currently on Model is that one is static and one is not, you're within copy-pasting distance of the solution (the trait DRY thing is a nice bonus, but it can come later if you get lost).

How does that change the code in Relation.php?

It wouldn't. All this work is done in PHPStan extensions. See the files in lib/PhpStan to get an idea. And note that these files are activated by pairing their names with their tags in phpstan.neon.dist.

I think the easiest way to resolve this is for you to do it this time and show me the diffs so I know what to do next time.

Easiest for you, maybe. 😄 But no worries, I'll take care of it this time, but in the future you might consider opening an issue before dropping more bombs like this one so we can identify these kind of issues first.

[shmax]

This is a bit awkward, isn't it? What are you fixing? If it's related to polluting the user's options with "order" in the case of "having", then you probably won't be surprised to hear that my advice is going to be to move that logic to Table::options_to_sql, and keep the rest of this clean.

[ipundit]

It's to:

1. Deny the user to be able to call Author::to_a($myHandCraftedOptionsThatWouldHaveToBeInputChecked)
2. Fix the bugs in find_by(), first(), last(), and take(). There are new tests in the test classes which trigger the bug of clobbering $options, but this submission fixes those bugs.

[shmax]

1. Deny the user to be able to call Author::to_a($myHandCraftedOptionsThatWouldHaveToBeInputChecked)

Is that a thing? There's no static to_a on Model and the to_a on Relation doesn't take any arguments. But let me take a look at your tests...

[shmax]

Ah, yep, those tests show the problem, for sure...

[shmax]

Maybe a bit simpler?

https://github.com/php-activerecord/activerecord/compare/how-about-this?expand=1

[ipundit]

    public function testDoesNotClobberOrderOrLimit()
    {
        $rel = Author::limit(2)->order('author_id DESC');
//        $this->assertEquals(1, $rel->first()->author_id);

The line should not be commented and the test must pass. first() is defined to return the first record in the table, despite order('author_id DESC'). Or should the spec be changed to?:

$author = Author::order('author_id DESC')->first();
$author2 = Author::order('author_id ASC')->last();

$this->assertEquals($author, $author2);

As for your other changes, clone $this would be less performant than the current implementation of cloning just $this->options rather than the entire $this object. Also, what does it mean to clone $this->generator and $this->table? If those classes have a database connection somewhere, or some kind of other resource where a shallow clone would be problematic, then this could lead to difficult to find bugs. I would rather only clone $this->options if I'm only changing $this->options and nothing else.

[shmax]

The line should not be commented and the test must pass. first() is defined to return the first record in the table, despite

Yes, yes, the tests work even with that line uncommented.

Also, what does it mean to clone $this->generator and $this->table

If the tests pass, then the tests pass. 😄

But if you don't like it, we can go with your code and I'll sort it out later.

[shmax]

Thanks!