test for finding by null #420

shmax · 2014-07-15T00:21:03Z

Fix for the following issue:

$author = Author::find(0);
// throws RecordNotFound exception (because count(0) == 1)

$author = Author::find(null);
// does not throw an exception (because count(null) == 0)

jpfuentes2 · 2014-07-15T13:02:36Z

I'm not sure I am convinced of the utility of this change. It seems to me that this is a user code error because one should not be searching for records in a RDBMS via a null primary key. I believe the proper behavior is to throw RecordNotFound or perhaps a specific error message on null?

shmax · 2014-07-15T18:16:46Z

I believe the proper behavior is to throw RecordNotFound

That's what I'm doing.

Added a custom exception message for invalid key argument.

I snooped around a bit in the rails code to see what they do. Someone who can read Ruby better than me will have to help, but it looks to me like they just do the standard RecordNotFound exception.

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/core.rb

shmax · 2014-07-15T18:49:11Z

I went poking into their tests, and the plot thickens, a bit:

https://github.com/rails/rails/blob/master/activerecord/test/cases/finder_test.rb

They have a test called "test_find_an_empty_array", which expects an empty array as a result. Strangely, they have another test called "test_find_by_empty_ids" which seems to do the same exact thing.

def test_find_by_empty_ids
    assert_equal [], Post.find([])
end

def test_find_an_empty_array
    assert_equal [], Topic.find([])
end

I don't see any tests for null being passed in, but apparently in at least some cases an empty result set can be returned.

jpfuentes2 · 2014-07-15T19:01:50Z

I believe we have the find_by_array equivalent in PHP-AR which returns an empty array. That is not the same behavior as a single $id or null.

shmax · 2014-07-15T19:04:56Z

@jpfuentes2
Is it safe to assume that you have a ruby environment set up? Could you be so kind as to see what happens when you find by null?

jpfuentes2 · 2014-07-15T19:07:08Z

Kinda, but I don't have any DB stuff setup. I went from PHP -> Ruby -> Scala so I don't do much Ruby these days either.

jpfuentes2 · 2014-07-15T19:09:35Z

I don't think it raises a specific exception. I think RecordNotFound is appropriate behavior here.

shmax · 2014-07-15T19:11:57Z

Groovy. Let me know if anything else seems suspicious.

jpfuentes2 · 2014-07-15T19:14:37Z

lib/Model.php

@@ -1581,19 +1581,23 @@ public static function find(/* $type, $options */)
 	 */
 	public static function find_by_pk($values, $options)
 	{
+		if(is_null($values)){


if (expr) { }

Or

if (expr) expr

null === $values is faster than is_null($values)

Should we be checking explicit null or empty here?

Well, we could check for empty, but then that will change the existing behavior for passing in "0" and empty array.

shmax · 2014-07-17T00:20:55Z

Rebased on master.

shmax · 2014-07-17T02:23:59Z

By the way, is there anything I can do to help get some of these older PRs up to date, passing tests, etc?

koenpunt · 2014-07-17T08:36:58Z

There are these long standing issues: #357 and #356 which list all PR's staged for a new release. I'd wanted to start a 1.1-dev branch for some time now to start merging these PRs. But have not found enough time yet..

koenpunt · 2014-07-17T08:49:28Z

Oh well, I just did it: https://github.com/jpfuentes2/php-activerecord/tree/1.1-dev

shmax · 2014-07-17T10:08:01Z

@koenpunt So how can I help?

koenpunt · 2014-07-17T11:05:32Z

Review any of the open todo's on #430 and submit a rebased version to the 1.1-dev branch

koenpunt · 2014-12-08T14:50:27Z

In Ruby a ActiveRecord::RecordNotFound is thrown when using nil:

irb(main):002:0> App.find(nil)
ActiveRecord::RecordNotFound: Couldn't find App without an ID

Same for 0, but does a lookup on the database first:

irb(main):004:0> App.find(0)
  App Load (0.4ms)  SELECT  `apps`.* FROM `apps`  WHERE `apps`.`id` = 0 LIMIT 1
ActiveRecord::RecordNotFound: Couldn't find App with 'id'=0

jpfuentes2 · 2014-12-08T15:15:14Z

I'm +1 for this but let's use Rails AR's message: Couldn't find #{Model} without an ID

koenpunt · 2014-12-08T15:21:26Z

lib/Model.php

@@ -1645,6 +1646,11 @@ protected static function get_models_from_cache(array $pks)
 	 */
 	public static function find_by_pk($values, $options)
 	{
+		if(is_null($values))
+		{
+			throw new RecordNotFound("Invalid primary key used (".var_export($values, true).")");


Using var_export here is unnecessary, you can simple use null because you're using is_null for testing.

shmax · 2014-12-08T16:34:10Z

Comments addressed.

koenpunt · 2014-12-08T16:54:22Z

lib/Model.php

@@ -887,7 +887,8 @@ private function update($validate=true)
 	protected function update_cache()
 	{
 		$table = static::table();
-		if($table->cache_individual_model){
+		if($table->cache_individual_model)
+		{


Can you revert this change? as it is not related to the actual changes. And because there are other changes pending for this part of the code.

cvanschalkwijk · 2014-12-08T20:42:30Z

I think it looks good. +1

koenpunt · 2014-12-08T20:44:09Z

@shmax can you squash your commits into 1? Thanks

shmax · 2014-12-09T04:57:46Z

Squashed to 1 commit.

jpfuentes2 · 2014-12-09T14:34:55Z

👍 thanks! Who wants the honor of merging?

koenpunt · 2014-12-09T16:20:55Z

lib/Model.php


 			if ($expected == 1)
 			{
-				if (!is_array($values))
-					$values = array($values);
-
 				throw new RecordNotFound("Couldn't find $class with ID=" . join(',',$values));


I'm still kinda puzzled by this. $expected == 1 so no need to join right? Only because it's converted to an array above.

Good catch: why is this moved? What effect does it have?

Well, this isn't my code, and I'm not aware of all the possible cases, but this seems to catch a wide variety of them. Imagine the case where an array with one item in it is passed in. Then count($values) == 1, the force to array doesn't happen, but join still works. If a single scalar value is passed in, count($values) is still 1, it gets forced to array, and the join still works.

Oh, and I probably moved it because line 1678 requires an array, but the operation to guarantee an array was happening inside a logical check. It may be that the move wasn't needed, but I found it more clear to read.

shmax · 2014-12-09T17:20:38Z

Per @koenpunt, consolidated two exceptions.

koenpunt · 2014-12-09T17:22:01Z

I don't think we should remove the second exception

shmax · 2014-12-09T17:24:52Z

Oh, I see what you mean, one sec...

shmax · 2014-12-09T17:27:50Z

Small rearrangement, re-squashed.

koenpunt · 2014-12-09T17:30:26Z

lib/Model.php

+			if (!is_array($values))
+				$values = array($values);
+
+			$values = join(',',$values);


Almost there, there is no need to convert values to an array if we're going to join it directly after. Or am I missing something?

if (is_array($values)) $values = join(',', $values);

shmax · 2014-12-09T17:32:22Z

sgtm

koenpunt · 2014-12-09T17:34:00Z

lib/Model.php

-					$values = array($values);
-
-				throw new RecordNotFound("Couldn't find $class with ID=" . join(',',$values));
+				throw new RecordNotFound("Couldn't find $class with ID=" . $values);


Final note, please use interpolation for $values as well, instead of concatenation.

shmax · 2014-12-09T17:54:19Z

Okie-doke. At this point I have to get to work, so if there is anything else you will either need to make the change yourself, or wait until I get back.

koenpunt · 2014-12-09T17:55:00Z

I think this will be just fine, thanks!

test for finding by null

jpfuentes2 reviewed Jul 15, 2014
View reviewed changes

koenpunt reviewed Dec 8, 2014
View reviewed changes

shmax force-pushed the find-null branch from 45f5c2f to 030b248 Compare December 9, 2014 04:56

koenpunt reviewed Dec 9, 2014
View reviewed changes

shmax force-pushed the find-null branch from 32e2584 to e504ad6 Compare December 9, 2014 17:27

koenpunt reviewed Dec 9, 2014
View reviewed changes

shmax force-pushed the find-null branch from e504ad6 to d1a924b Compare December 9, 2014 17:31

koenpunt reviewed Dec 9, 2014
View reviewed changes

fix find by null

29c04fc

shmax force-pushed the find-null branch from d1a924b to 29c04fc Compare December 9, 2014 17:52

koenpunt added a commit that referenced this pull request Dec 9, 2014

Merge pull request #420 from shmax/find-null

083c446

test for finding by null

koenpunt merged commit 083c446 into jpfuentes2:master Dec 9, 2014

test for finding by null #420

test for finding by null #420

Conversation

shmax commented Jul 15, 2014

jpfuentes2 commented Jul 15, 2014

shmax commented Jul 15, 2014

shmax commented Jul 15, 2014

jpfuentes2 commented Jul 15, 2014

shmax commented Jul 15, 2014

jpfuentes2 commented Jul 15, 2014

jpfuentes2 commented Jul 15, 2014

shmax commented Jul 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmax commented Jul 17, 2014

shmax commented Jul 17, 2014

koenpunt commented Jul 17, 2014

koenpunt commented Jul 17, 2014

shmax commented Jul 17, 2014

koenpunt commented Jul 17, 2014

koenpunt commented Dec 8, 2014

jpfuentes2 commented Dec 8, 2014

Choose a reason for hiding this comment

shmax commented Dec 8, 2014

Choose a reason for hiding this comment

cvanschalkwijk commented Dec 8, 2014

koenpunt commented Dec 8, 2014

shmax commented Dec 9, 2014

jpfuentes2 commented Dec 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmax commented Dec 9, 2014

koenpunt commented Dec 9, 2014

shmax commented Dec 9, 2014

shmax commented Dec 9, 2014

Choose a reason for hiding this comment

shmax commented Dec 9, 2014

Choose a reason for hiding this comment

shmax commented Dec 9, 2014

koenpunt commented Dec 9, 2014