Report templates cloned from those created on XUL client causing error or producing different results

[[e-mail]]

This is related to fix in bug 1642344.

We deployed the fix and started to see issues.

Cloning process seems fine. Without updating anything the template can be saved.

When running reports from the cloned templates, there were errors complaining of either lacking field in Group By or missing From clause.

When cloning the templates, removing then putting back the complained fields solved the issue (report can run).

Some cloned templates seem working fine. But the results do not match those generated from the XUL templates.

It seems the issue is related to how the tables are joined. On the report interface, I do not see join type on templates cloned from XUL templates. But if I replace a field, I see join type. When I create a template from scratch I see the join type when choosing a field from a linked table.

[[e-mail] (tji)]

Attached are two examples templates in 3 states: before being clone, cloned without editing, cloned with replaced fields from linked tables (following the same path).

[[e-mail] (tji)]

another example

[Anna Goben (agoben)]

I can confirm we're seeing the same problems with any join that wasn't an INNER for cloned templates.

[Jason Boyer (jboyer)]

This branch tries to address this by choosing left joins for any relationship types other than has_a (which is supposed to be inner anyway). In my testing it allowed correct XUL->Web report conversion and also helped correct an issue I was having (related to might_have links and field names != 'id').

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jboyer/lp1796945_report_joins / working/user/jboyer/lp1796945_report_joins

[Jason Boyer (jboyer)]

That should have read "in my limited testing..." Anyone that's interested please test thoroughly in case I've missed some glaring issue.

[Terran McCanna (tmccanna)]

We have applied this code and haven't seen any issues, but we haven't identified any particular reports we had been having problems cloning yet, so not comfortable signing off. Tina, are you able to apply Jason's code to see if it fixes the problems with your reports?

[[e-mail] (tji)]

We observed two types of issues with templates cloned from XUL:

1. error when running reports
2. inaccurate result

The fix helps with #2, but does not solve #1.

For #1, we usually see two types of errors: one is missing FROM clause, the other a field needs be in GROUP BY. Below is an example of the latter from today's test. I did not encounter the former today.

DBD::Pg::st execute failed: ERROR: column "f45381a54504218e39aca33492d29306.circ_lib" must appear in the GROUP BY clause or be used in an aggregate function LINE 8: HAVING "f45381a54504218e39aca33492d29306"."circ_lib" IN ($... ^ at /srv/openils/bin/clark-kent.pl line 255.

By comparing the cloned template with a new template directly created on the Webby, I noticed that the cloned template has an empty "where" section, but have the conditions in "having" section (copied an example portion below). Moving the conditions to "where" section solved the problem.

"where":[],"having":[{"alias":"Circulating Library","path .......

Note that I have at least one cloned template that has both "where" and "having" sections, but causes the GROUP BY error. It could be related to individual fields in each sections.

Testing on #2 shows positive result. I chose a template that relies on the hidden left join from Invoice table to Invoice Entry and to Invoice Items. The cloned template with Jason's fix produced the same result as the XUL template. But the one cloned without the fix produced a shorter result list.

More testing will be on the missing FROM clause error and impact of Jason's fix on templates having right join (unlikely an issue).

Tina Ji
BC Libraries Coop

[[e-mail] (tji)]

An example that a right join in XUL template was changed to inner join during the cloning.

The XUL template (action.in_house_use right join asset.copy):

"from":{"path":"aihu-item","table":"action.in_house_use","label":"In House Use","alias":"e443e095dd88083d1a9da7e3630b7185","idlclass":"aihu","template_path":"aihu","join":{"item-e443e095dd88083d1a9da7e3630b7185":{"key":"id","type":"right","path":"aihu-item-acp-circ_lib","table":"asset.copy","alias":"67bf5c5e1a824cdc5d44508d00cf2ff7","join":{"id-erfcc-id-20caf18a507c84af49058a17e5f1bc79":{"key":"id","type":"inner","path":"

After the cloning, it became inner join (with the fix on, but I assume the fix is not expected to have impact on this part.)

"from":{"alias":"e443e095dd88083d1a9da7e3630b7185","path":"aihu-aihu","table":"action.in_house_use","idlclass":"aihu","label":"In House Use","join":{"item-156238d5825ba00cfa7fe9ec1979a4c9":{"type":"inner","key":"id","alias":"67bf5c5e1a824cdc5d44508d00cf2ff7","path":"aihu-item","table":"asset.copy","idlclass":"acp","join

[Dan Wells (dbw2)]

I think the branch posted has the right idea, but as noted, is not going to be right in some cases. Even 'has_a' doesn't necessarily mean an inner join if the data allows the link to be null. (has_a only determines "direction" of the link)

Ultimately we're going to need to pull out the defined joins from the old structure if we really want to do a true conversion, since even if we improve the default logic, the old interface allowed one to override that default and set the join manually. Pulling out the old data didn't seem obvious after looking at least one bug back, but I am not yet familiar with the structural changes.

[Dan Wells (dbw2)]

A variety of improvements to the webstaff reporter interface are now available here:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1796945_reporter_clone_create_fixes

working/user/dbwells/lp1796945_reporter_clone_create_fixes

I am hoping to carve out time for automated test cases, but didn't want this lingering in an unavailable state.

From the commit message:

This commit addresses a variety of issues with the webstaff reporter interface, particularly cases of cloning reports created in the XUL client.

1. The conversion process did not account for manually selected JOIN operations (aka nullability). These JOINs are now honored by the conversion code.

2. The conversion process did not account for aggregate filters. These filters are now converted where present.

3. The previous reporter interface attempted to intelligently apply LEFT and INNER JOINs by default. The new interface applied INNER joins exclusively by default, leading in many cases to different results. This commit reinstates the previous logic (which was partially present but never triggered). One side effect of this change is that the IDL tree itself is no longer opinionated about JOIN type, and the default JOIN is undefined.

4. The nullability selector has been expanded to allow for manual selection of INNER joins, as they will longer be the default in some cases.

5. Cloned-converted reports did not retain column order. The order is now preserved.

6. Some templates created in the older interface could, in some cases, have aggregate values set as the string "undefined" rather than actually being undefined. This led to converted templates failing with "column [xxx] must appear in the GROUP BY clause...", as they were incorrectly converted as aggregates. The conversion code now accounts for this latent bug.

[[e-mail] (tji)]

We deployed the fix on a 3.1.7 test server. I picked some XUL templates to test all the listed scenarios. They worked for me. For

Tina Ji
BC Libraries Coop

[[e-mail] (tji)]

I encountered an issue when adding a displayed or filter field from a linked table, I got the missing FROM clause error.

Example 1: starting from In-house Use, right join Item to list items with total circ count and in-house use count. The cloned template works. But I got the error when cloning it again from the cloned template to add Active Date from Item table. I got the same error when adding the field during the initial cloning from the XUL template, too.

Example 2: template start from Item to list items by circ library. I got the error when I added a filter on item's shelving location (on location id in shelving location table (use the default inner join)).

I also tested creating the templates from scratch on Webby, then clone to add the fields. No error.

[Jason Boyer (jboyer)]

Thanks Dan, your branch is a great improvement. I think I can also see what's going on in Tina's examples too. The Source Path separator is different between the XUL and web clients: The XUL Client uses both '->' and '::' depending on how many layers down the tree you go, while the Web Client instead only uses '->' regardless of the number of links traversed. So when you clone a simple XUL report that results in this SQL:

SELECT "67bf5c5e1a824cdc5d44508d00cf2ff7"."barcode" AS "Barcode",
    COUNT(DISTINCT "e443e095dd88083d1a9da7e3630b7185"."id") AS "Use ID",
    "076bf6f3642cbc96b197e17f5bc8943e"."circ_count" AS "Total Circulation Count"
FROM action.in_house_use AS "e443e095dd88083d1a9da7e3630b7185"
    RIGHT OUTER JOIN asset.copy AS "67bf5c5e1a824cdc5d44508d00cf2ff7" ON ("e443e095dd88083d1a9da7e3630b7185"."item" = "67bf5c5e1a824cdc5d44508d00cf2ff7"."id")
    FULL OUTER JOIN extend_reporter.full_circ_count AS "076bf6f3642cbc96b197e17f5bc8943e" ON ("67bf5c5e1a824cdc5d44508d00cf2ff7"."id" = "076bf6f3642cbc96b197e17f5bc8943e"."id")
WHERE "67bf5c5e1a824cdc5d44508d00cf2ff7"."circ_lib" IN ($_15939$335$_15939$)
GROUP BY 1, 3, 4
ORDER BY "67bf5c5e1a824cdc5d44508d00cf2ff7"."barcode" ASC, COUNT(DISTINCT "e443e095dd88083d1a9da7e3630b7185"."id") ASC, "076bf6f3642cbc96b197e17f5bc8943e"."circ_count" ASC

in the web client and try to add the Active Date field (following the same right join path, as expected) you'll still end up with something like this:

SELECT "67bf5c5e1a824cdc5d44508d00cf2ff7"."barcode" AS "Barcode",
    COUNT(DISTINCT "e443e095dd88083d1a9da7e3630b7185"."id") AS "Use ID",
    "076bf6f3642cbc96b197e17f5bc8943e"."circ_count" AS "Total Circulation Count",
    "92632112b9e5a769d2fa4b1c967657f5"."active_date" AS "Active Date/Time"
FROM action.in_house_use AS "e443e095dd88083d1a9da7e3630b7185"
    RIGHT OUTER JOIN asset.copy AS "67bf5c5e1a824cdc5d44508d00cf2ff7" ON ("e443e095dd88083d1a9da7e3630b7185"."item" = "67bf5c5e1a824cdc5d44508d00cf2ff7"."id")
    FULL OUTER JOIN extend_reporter.full_circ_count AS "076bf6f3642cbc96b197e17f5bc8943e" ON ("67bf5c5e1a824cdc5d44508d00cf2ff7"."id" = "076bf6f3642cbc96b197e17f5bc8943e"."id")
WHERE "67bf5c5e1a824cdc5d44508d00cf2ff7"."circ_lib" IN ($_2397$335$_2397$)
GROUP BY 1, 3, 4
ORDER BY "67bf5c5e1a824cdc5d44508d00cf2ff7"."barcode" ASC, COUNT(DISTINCT "e443e095dd88083d1a9da7e3630b7185"."id") ASC, "076bf6f3642cbc96b197e17f5bc8943e"."circ_count" ASC, "92632112b9e5a769d2fa4b1c967657f5"."active_date" ASC

Where the hash "92632112b9e5a769d2fa4b1c967657f5" is assumed to be the same path as hash "67bf5c5e1a824cdc5d44508d00cf2ff7" (because it is, except for the difference in separators) and so isn't defined anywhere, leading to a FROM clause complaint.

This leads to a situation where it's possible to clone a XUL report and manipulate it in the web client, but actually adding new fields to the reports is highly unlikely to ever succeed.

I'm open to suggestions on how to proceed here, should there be a key dropped in the JSON that triggers the old "source -> path :: path" style of source specifier for reports of that era, or should all of the hashes be re-generated every time a report is saved (I haven't checked ...

[Dan Wells (dbw2)]

I think I have a simple fix for this, but running late for something. Will post in the morning!

[Dan Wells (dbw2)]

Okay, small change now pushed:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1796945_reporter_clone_create_fixes

working/user/dbwells/lp1796945_reporter_clone_create_fixes

Please see the commit message for more info, but suffice it to say that Jason B. had it right.

[Jason Boyer (jboyer)]

I've tried out Dan's updated branch above and it works great. Signoff is here: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jboyer/lp1796945_signoff / working/user/jboyer/lp1796945_signoff

[Jason Boyer (jboyer)]

I should have looked at the calendar to get this into yesterday's releases but that didn't work out.

Thanks Tina and Dan!