statamic / eloquent-driver

Provides support for storing your Statamic data in a database, rather than flat files.
https://statamic.dev/tips/storing-content-in-a-database
MIT License
104 stars 73 forks source link

Ensure it is possible to order entries by integer, float or date fields #154

Closed helloiamlukas closed 1 year ago

helloiamlukas commented 1 year ago

fixes https://github.com/statamic/eloquent-driver/issues/153

what-the-diff[bot] commented 1 year ago

PR Summary

ryanmitchell commented 1 year ago

This solution is MySQL specific, so that would mean it's not something we could incorporate into a release. Im very open to finding an alternative way to sort this, especially if we can apply it to other column types too.

By the way, as a work around, you could make a migration to create a stored/generated field in MySQL with the value of the column and the data type you want. MySQL is smart enough to use that column for performance reasons even when you use data->xxx.

helloiamlukas commented 1 year ago

What do you mean by this solution being MySQL specific? This solutions applies to PostgreSQL and SQLite too, or am I missing something? We use this fix for PostgreSQL in production for a while now without any problems.

ryanmitchell commented 1 year ago

Sorry, wrong way round. I should have said it doesn't work in MySQL.

In MySQL the syntax would need to be orderByRaw("data->\$.'{$column}' {$direction}") otherwise you get an "invalid JSON path expression" error.

SQLite seems to use the same syntax: https://www.sqlite.org/json1.html#jptr

Am I missing something?

helloiamlukas commented 1 year ago

Oh, I see the problem now. And I am with you that there should probably no MySQL/Postgres/SQLite specific code included in the query builder.

One solution that came across my mind: We could extend the grammars and add a method compileFloatValueCast. This could be used in the query builder then. I'm not sure if this is the right approach though.

ryanmitchell commented 1 year ago

It's a hard one to solve while not making it specific to a certain database engine. My current thinking is we need to CAST the value as a column type (I think this is generic to all 3 engines). Your use case is numeric, but the same issue exists for dates so it needs to be solved in a non-specific way.

ryanmitchell commented 1 year ago

@helloiamlukas ok, heres a change of approach to use CAST(x as y) where available. SQLite messes things up a little bit so I've hard coded a change for it on dates, which isnt ideal, but we can revisit that if it causes issues down the line.

Let me know your thoughts on this.

helloiamlukas commented 1 year ago

Looking good to me, thank you :)

One more thing we should maybe consider: Text fieldtypes can have a number or date input. Should we also cast text fieldtypes when the input_type is number or date?

ryanmitchell commented 1 year ago

I think I'm happy enough (for now) to say that you have to use one of the explicit field types... we can always revisit that later down the road if that needs to change.

Thanks for your help!