fuel / orm

Fuel PHP Framework - Fuel v1.x ORM
http://fuelphp.com/docs/packages/orm/intro.html
151 stars 95 forks source link

Orm remembers which fields were selected previously when it shouldn't #427

Closed paullb514 closed 4 years ago

paullb514 commented 4 years ago

Orm remembers which fields were selected previously when it shouldn't. In $result2 below no fields were specified so one would expect that it gets the full row. However, it "remembers" what was fetched previously. This can lead to unpredictable results based on the order of execution. The Query should be stateless.

$result1 = \Model_X::query()->select('id')->get();
$result2 = \Model_X::find(1);

Expected: All fields from Model_X should be included in $result2. Acutal: Only ID is included in $result2

One would then expect to guarantee that I will get all fields I need to explicitly list them all in a second select statement. However, even this doesn't seem to work and I still only get the id back. This "select" function seems thoroughly broken upon repeated usages

\Model_X::query()->select('col2')->where('id', 1);
WanWizard commented 4 years ago

This is by design. The ORM is not a query builder, you should not use it as such, use DB calls.

The ORM caches result objects, exactly to make sure that

$result1 = \Model_X::find(1);
$result2 = \Model_X::find(1);
$result1 === $result2; // true

This means selects should not be used, or used very sparingly, giving this behaviour. If I recall the current develop code has features to update existing objects, but I would still not give any guarantee.

If you want to do this, use

\Model_X::query()->select('col2')->where('id', 1)->from_cache(false)->execute();

do avoid repopulation from cache.

paullb514 commented 4 years ago

There are three issues here: 1) Then this would seem broken in the form of asking for different columns should not hit the cache. This is obviously a bug as what is being asked for is not being returned. 2) ORM is obviously a query builder as it exposes all sorts of query building functions under the Model_X::Query set. 3) Checking the DB Class documentation there is no support for relations set in ORM.

Either way, this is obviously a bug and not by design.

WanWizard commented 4 years ago

No, this is by design. Don't use select() if you don't undnerstand its behaviour.

If you don't like it, you can disable ORM object caching globally, see https://github.com/fuel/orm/blob/1.8/master/config/orm.php#L15 (but expect a performance hit).

WanWizard commented 4 years ago

See also the docs: https://fuelphp.com/docs/packages/orm/intro.html#/caching

paullb514 commented 4 years ago

The function does not work as one would expect reading the documentation ( https://fuelphp.com/docs/packages/orm/crud.html). Either the design needs to be fixed or the documentation updated, but either way, this does not work as one would expect. If reading the documentation doesn't let one "understand the behaviour" then the documentation is incomplete.

Specifying what fields should retrieved and it them not being returned is unexpected behaviour (a bug) however one looks at it.

WanWizard commented 4 years ago

The "crud" page just gives coding examples, it isn't about caching. You can argue as much as you want, caching is not going to be changed.

And, as I have already mentioned before, the select() behaviour has already been fixed:

$user1 = \Model\User::query()->select('id')->where('id', 1)->get_one();
var_dump($user1);
object(Model\User)[113]
  protected '_is_new' => boolean false
  protected '_frozen' => boolean false
  protected '_sanitization_enabled' => boolean false
  protected '_data' => 
    array (size=11)
      'id' => string '1' (length=1)
      'username' => null
      'password' => null
      'group' => null
      'email' => null
      'last_login' => null
      'login_hash' => null
      'profile_fields' => null
      'user_id' => null
      'created' => null
      'updated' => null
  protected '_custom_data' => 
    array (size=0)
      empty
  protected '_original' => 
    array (size=11)
      'id' => string '1' (length=1)
      'username' => null
      'password' => null
      'group' => null
      'email' => null
      'last_login' => null
      'login_hash' => null
      'profile_fields' => null
      'user_id' => null
      'created' => null
      'updated' => null
  protected '_data_relations' => 
    array (size=0)
      empty
  protected '_original_relations' => 
    array (size=0)
      empty
  protected '_reset_relations' => 
    array (size=0)
      empty
  protected '_disabled_events' => 
    array (size=0)
      empty
  protected '_view' => null
  protected '_iterable' => 
    array (size=0)
      empty
$user2 = \Model\User::query()->where('id', 1)->get_one();
var_dump($user2);
object(Model\User)[113]
  protected '_is_new' => boolean false
  protected '_frozen' => boolean false
  protected '_sanitization_enabled' => boolean false
  protected '_data' => 
    array (size=11)
      'id' => string '1' (length=1)
      'username' => string 'test' (length=4)
      'password' => string 'BsO7Rn++MXbWMrJK7GrJ0Jh19HbWYrM+WjxCw4d79/0=' (length=44)
      'group' => string '100' (length=3)
      'email' => string test@example.org' (length=16)
      'last_login' => string '1572969920' (length=10)
      'login_hash' => string 'ea307866ed2d3139c081f2e49bdb202ac8e49962' (length=40)
      'profile_fields' => string 'a:0:{}' (length=6)
      'user_id' => string '1' (length=1)
      'created' => string '1360405804' (length=10)
      'updated' => string '1361803501' (length=10)
  protected '_custom_data' => 
    array (size=0)
      empty
  protected '_original' => 
    array (size=11)
      'id' => string '1' (length=1)
      'username' => string 'test' (length=4)
      'password' => string 'BsO7Rn++MXbWMrJK7GrJ0Jh19HbWYrM+WjxCw4d79/0=' (length=44)
      'group' => string '100' (length=3)
      'email' => string test@example.org' (length=16)
      'last_login' => string '1572969920' (length=10)
      'login_hash' => string 'ea307866ed2d3139c081f2e49bdb202ac8e49962' (length=40)
      'profile_fields' => string 'a:0:{}' (length=6)
      'user_id' => string '1' (length=1)
      'created' => string '1360405804' (length=10)
      'updated' => string '1361803501' (length=10)
  protected '_data_relations' => 
    array (size=0)
      empty
  protected '_original_relations' => 
    array (size=0)
      empty
  protected '_reset_relations' => 
    array (size=0)
      empty
  protected '_disabled_events' => 
    array (size=0)
      empty
  protected '_view' => null
  protected '_iterable' => 
    array (size=0)
      empty

As you can see, object id's are the same, missing column values have been added.

var_dump($user1 === $user2);
APPPATH/classes/controller/homepage.php:38:boolean true
WanWizard commented 4 years ago

So either wait until the next release, or switch (the ORM package) to 1.9/develop if you can't wait.

paullb514 commented 4 years ago

Great that this issue has already been addressed. (you could have just said so from the beginning instead of insisting that the faulty behaviour was "by design")

I'm happy enough to wait until everything is stable enough to get out :)

Thank you for the time going through this

WanWizard commented 4 years ago

There is a difference between something being by design, and something being logical :grin:

Object caching was part of the original design by Jelmer, now 10 years ago, and the first versions didn't even have a select() method. It was added later to reduce the size of the objects to aid things like reporting, where lots of objects need to be retrieved, but not all columns, thus reducing the memory requirements of the app. But with the caching caveat. Later again, code was added to enable/disable the cache, deal with this issue.

A few months ago, I completely rewrote the object hydration code, which seems to have, as an unintended side-effect, addressed this issue :smiley:.