flatten bindings for Telescope

[macbookandrew]

trafficstars

Once https://github.com/laravel/telescope/pull/1499 is merged, this will cause an error due to the deeper array syntax that FM queries use.

Before, the bindings result in this shape:

[
    ['kp_id' => 'ABC123'],
    ['kp_id' => 'ABC124'],
    ['kp_id' => 'ABC125'],
];

After:

[
    'kp_id: ABC123',
    'kp_id: ABC124',
    'kp_id: ABC125',
];

[Smef]

Is this change necessary still or was it resolved on the other side?

[macbookandrew]

Still need this in order to work with Telescope

[tkuijer]

This exception is also something we encountered in our projects. In our case the structure of the bindings caused the new Laravel exception page to fail rendering with an 'array to string conversion' error.

[Smef]

Can you give me some information for how to reproduce this? I've thrown exceptions in queries from things like fields missing and it all displays properly. I'm testing on Laravel 11.21.0

Also, it looks like the other PR was merged in which seemed like the goal was to handle the structure here.

[Smef]

Aaaahh. I see now. Exception problem is only an issue if Telescope is installed, not in general. I've reproduced this now.

[Smef]

It looks like this issue may be something that needs to be fixed in telescope. The bindings property is supposed to be an array, not a string. It seems like changing this would result in improperly typing this parameter.

array	$bindings	The array of query bindings.

https://laravel.com/api/11.x/Illuminate/Database/Events/QueryExecuted.html

[macbookandrew]

The bindings property is supposed to be an array, not a string.

Correct. This PR is still passing an array to $bindings, it’s just flattening it a bit from array<int, array<string, string>> to array<int, string>.

Telescope loops over that $bindings array here, passing each to the quoteStringBinding() method, which expects each to be a string, rather than an array. That’s why I’m suggesting changing ['kp_id' => 'ABC123'] to 'kp_id: ABC123'

[macbookandrew]

This is due to the syntax for FM find requests compared to other database engines:

FM expects an object for each field:

{
  "query":[
    {"Group": "=Surgeon"},
    {"Work State" : "NY", "omit" : "true"}
  ]
}

where other database engines would use a key/value pair for each field:

// pseudo-code
{
  "query":[
    "Group": "Surgeon",
    "Work State" : "NY"
  ]
}

…and it occurs to me that we probably should keep omit or anything else in that object…just a minute and I’ll push up a fix for that 🤦🏻

[Smef]

The reason these are objects is because they're separate find requests, so just flattening them down into the same object destroys the "or" logic of multiple finds. I think some additional structure changes might be needed to support that as well.

[macbookandrew]

So we could flatten that example to this:

[
    "Group: =Surgeon",
    "Work State: NY, omit: true",
]

Would that work? or do you have a more detailed example of multiple OR fields query?

[Smef]

Omit is definitely an issue, since the omit is for the entire find request, and not the individual binding. The following binding definitely isn't correct to reflect the search query:

[
    "Group: =Surgeon",
    "Work State: NY, omit: true",
]

In this case, it's omitting results which match the Work State AND group.

Where are these bindings viewed, though? I see the "query" which still shows the actual FileMaker Data API query correctly. Clockwork shows it correctly as well and does not show the bindings.

Maybe flattening doesn't matter? I wouldn't want to break the correct FM Query which shows up in the log, but I don't think changing this affects that either here or in Clockwork.

[Smef]

Another thing to consider: Flattening this would cause multiple queries which search on the same field to overwrite the binding values of previous searches. The actual "SQL" query part being logged seems like it would still show the correct info, but the binding information would all be wrong.

[Smef]

One thing that could help with not overwriting the bindings is to prefix each binding key with the index, so that the original queries could potentially be rebuilt.

Ex:

"0 - name": "Zuko",
"0 - type": "Bird",
"1 - name": "Yoshi",
"1 - type" : "Bird",
"1 - omit" : "true",

Or something along those lines.

[Smef]

@macbookandrew can you make a change to identify the request number as part of the flattening? That'll keep the bindings making sense, though they don't really apply to FM requests in the same way.

[macbookandrew]

Given this query:

TimeEntry::query()
    ->where('date', '>=', now()->subWeek()->format('m/d/Y'))
    ->where('contact_id_worker', User::first()->filemaker_contact_id)
    ->orWhere('contact_id_worker', User::find(13)->filemaker_contact_id)
    ->whereNot('contact_id_worker', User::find(12)->filemaker_contact_id)
    ->limit(10)
    ->orderByDesc('id')
    ->get();

Here is the query logged in Telescope, using the changes on this branch:

[Smef]

That part you're looking at is actually the "sql string" part of the event (which we just use to show API query info), not the bindings, which is why it still shows properly. I don't actually see the bindings appearing anywhere in the UI, interestingly.

[macbookandrew]

Huh…interesting. How do these changes show up in an app with Clockwork?

Here’s why it fails with Telescope’s QueryWatcher.php:

So even though Telescope is displaying the SQL string, it’s still parsing the bindings.

[Smef]

I didn't see the bindings in Clockwork either. It's just the sql query string that shows as well.

[macbookandrew]

In that case, do you want me to change anything here?

[Smef]

I think having the bindings still make sense would be good, so indicating which request they're from would prevent the data loss, even though it doesn't seem that important right now in our initial testing. Can you make the change to show the request index in each binding as mentioned above?

[macbookandrew]

Sure. Would you prefer this?

[
  "0: Date: >=08/30/2024, _kf_ContactIDWorker: 31195",
  "1: _kf_ContactIDWorker: 17159",
  "2: omit: true, _kf_ContactIDWorker: 10956"
]

or this?

[
  "0: Date: >=08/30/2024",
  "0: _kf_ContactIDWorker: 31195",
  "1: _kf_ContactIDWorker: 17159",
  "2: omit: true",
  "2: _kf_ContactIDWorker: 10956",
]

[Smef]

I think the array is supposed to be key-value pairs, not just an array of strings, but the second option is closer

[macbookandrew]

Here’s a similar MySQL query with bindings:

"select * from `time_logs` where `spent_on` >= ? and `user_id` = ? or `user_id` = ? and not `user_id` = ? order by `id` desc limit 10"

[
    0 => '2024-08-30',
    1 => 1,
    2 => 13,
    3 => 12,
]

And here’s the SQL and bindings produced by 5b79bb7:

"select
Method: post
URL: https://myfilemakerdatabase.example.com/fmi/data/vLatest/databases/DatabaseName/layouts/TimeEntry/_find
Data: {
    "query": [
        {
            "Date": ">=08\/30\/2024",
            "_kf_ContactIDWorker": "31195"
        },
        {
            "_kf_ContactIDWorker": "17159"
        },
        {
            "omit": "true",
            "_kf_ContactIDWorker": "10956"
        }
    ],
    "sort": [
        {
            "fieldName": "__kp_TimeEntryID",
            "sortOrder": "descend"
        }
    ],
    "limit": 10
}"

[
    0 => "0: Date: >=08/30/2024",
    1 => "0: _kf_ContactIDWorker: 31195",
    2 => "1: _kf_ContactIDWorker: 17159",
    3 => "2: omit: true",
    4 => "2: _kf_ContactIDWorker: 10956",
]

[Smef]

I think it can go either way, depending on how your bindings are done: https://www.php.net/manual/en/pdostatement.bindvalue.php

If you use '?' for your bindings it's just sequential in an array. If you use named bindings then you use key-value pairs. I think the "named bindings" methodology makes more sense for FM stuff since we do need to map to fields, and it would be easier for us to separate the field names and values for the purposes of rebuilding the queries from the bindings. it also makes reading all of that easier and is closer to the actual query being made.

[macbookandrew]

Makes sense. Updated results for 80fd004:

[
    "0: Date" => ">=08/30/2024",
    "0: _kf_ContactIDWorker" => "31195",
    "1: _kf_ContactIDWorker" => "17159",
    "2: omit" => "true",
    "2: _kf_ContactIDWorker" => "10956",
]

[Smef]

released as 2.4.4

[macbookandrew]

Awesome; thanks!