fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
802 stars 335 forks source link

ORM count() returns incorrect row number #2196

Closed glOOmyART closed 4 months ago

glOOmyART commented 9 months ago

this is with fuel 1.8.2 on PHP 7.4

don't know if it's a bug or if i'm doing something wrong
when i chain ORM::query() with related() and have any condition in the chain or in the model, the $query->count() returns a wrong number of rows
it also has some problems sometimes without relations but always when there is a condition somewhere, either in the chain or in the model
it is especially annoying in combination with pagination since it won't compile correctly with the wrong total_items

e.g.

$query = \Model::query()
->related(array(
  'related1',
  'related2',
))
->where('related1.related_id',$id)
->or_where('related2.related_id',$id)
->group_by('id')
->order_by('id','asc');

(both related models are a has_many relation)

when i execute $query->count() it returns that there is only one row in the result but actually there are 22 when i execute count($query->get())

when i remove the group_by, $query->count() returns the correct row count but actually has more rows in the dataset cause some are doubled, so on the last page of pagination it repeats until per_page threshold is reached

WanWizard commented 9 months ago

This is about the ORM, and as said many times, the ORM is not a query builder, it behaves differntly, so don't treat it as such.

With ORM queries the returned dataset isn't relevant, as it will be hydrated into ORM objects in a hierarchical structure. count() returns the number of (in your example) Model objects will be created, not how many records the underlying query returns.

If it returns 1, and it returns 22 with the group_by removed, it suggests all 22 records have the same id?

What is the original query, because one is manually altered and doesn't make sense now. What for example is "related_id' for column in both related1 and related2?

group_by is hardly ever, if at all, relevant in an ORM query. In this case, the result with and without it should be the same, as there will only ever be one Model object with a given id, no matter how many where in the raw query result.

glOOmyART commented 9 months ago

the parent model returns songs and the related models are instruments and production roles
the related_id is the id of a person and a person can have credits on a song for instruments played and/or production roles (e.g. producer, engineer, etc)
and i want to get all the songs a person worked on
since i don't want to list tracks twice i group them by their id

WanWizard commented 9 months ago

You can never get them twice, there is always only one model object per primary key?

Even if you have one song where one persion has 25 roles, there is still only one Song object in the result.

It seems to be you're still thinking in SQL, which isn't relevant here.

WanWizard commented 9 months ago

Just to be clear, you have

song -> has_many -> instrument -> belongs_to -> person
song -> has_many -> role -> belongs_to -> person

and the question is "give me all songs person X has played or worked on"?

In which case your code is absolutely fine, without the group_by() which is not needed.

glOOmyART commented 9 months ago

exactly that!

so my logic wasn't completely iffy
and yes, without group_by the $query->count() returns the correct number but messes up the pagination

depending on the items it returns, the pagination sometimes only shows two or three of them per page or starts over on the last page?
only when i set the per_page to the same number as count() it shows all records but that makes paging obsolete (obviously)

so currently, the pagination only displays and works correctly when i use group_by and set total_items to count($query->get())

i'm still not sure if it's my code or the framework but i think i'll stick with my workaround for now since i'm not in good enough shape to dig or elaborate deeper into the issue - it just needs to work

edit: thank you very much for taking the time and sry if i "missused" the bug tracker

WanWizard commented 9 months ago

If it messes up the pagination, you're doing that wrong too, or you are misusing the ORM.

If you want a page that looks something like this:

song - instrument - person
           instrument - person
           instrument - person
           instrument - person
           role - person
           role - person

you can't run an ORM query on songs as that returns only one object (which equates to one line) and use pagination, as pagination expects a fiat result ( 1 row = 1 record ).

You could get the data by starting with person, relate "role", "instrument", "role.song" and "instrument.song", but then you can't order by song as person is your main object now.

This is a typical example that shows that you should not use the ORM as a query builder, you can very easily do this with DB::select() without any of the problems you encounter, and witll a much smaller footprint.

It is also related to the fact you're using ORM models directly in your code (which makes it easy to make this mistake), while you should see it as a database layer abstraction.

Instead, you should build logical models with methods returning data structures to your application, and have these methods use either ORM or DB statements, based on requirements.

glOOmyART commented 9 months ago

the page should look like this

song
--- all other song information ---
--- people involved
  person1: instrument, instrument, instrument
  person2: instrument, instrument
--- the same with roles ---

while you should see it as a database layer abstraction.

that's how i understood it from the beginning

but with the possibility to add condition, it's sooo convenient to use ORM to also filter records
i'm also aware of the overhead (especially when using relations)

so, you suggest i should create a simple e.g. \Model\Searchperson class, put straight SQL queries in there and return the result to be used in the controller? often thought about that but as i said before, ORM is so convenient

but i think there's really no way around that

thanks again for your time, much appreciated!

WanWizard commented 9 months ago

If you compare, there isn't much difference in syntax, the only thing is that you need to write out your joins.

You can prevent hardcoded tables by using something like


->from(array(Model\Session::table(), 's'))
glOOmyART commented 9 months ago

wow, that's the greatest hint! thank you very much!
i didn't know or realize it could be done that way

so i can still use information from the ORM - that saves a lot of work when something changes

that's the drawback of autodidactic; some concepts or simpler solutions completely pass by unbeknownst and only come up when something gets too complex or starts to act unexpected