Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't make report from query saved in production #6022

Open
emenslin opened this issue Dec 31, 2024 · 2 comments · May be fixed by #6144
Open

Can't make report from query saved in production #6022

emenslin opened this issue Dec 31, 2024 · 2 comments · May be fixed by #6144
Assignees
Labels
1 - Bug Incorrect behavior of the product 2 - Queries Issues that are related to the query builder or queries in general regression This is behavior that once worked that has broken. Must be resolved before the next release.
Milestone

Comments

@emenslin
Copy link
Collaborator

Describe the bug
If you save a query in production (either make a new one or edit an old one) it throws an error if you try to make a report from it.

To Reproduce
Steps to reproduce the behavior:

  1. Go to queries
  2. Make report from existing query
  3. See it works
  4. Edit the query
  5. Try to make a report
  6. See error

Expected behavior
Should be able to make a report

Screenshots

yxj2fUElZp.mp4

Crash Report

Specify 7 Crash Report - 2024-12-31T15_46_12.430Z.txt

Please fill out the following information manually:

@emenslin emenslin added 1 - Bug Incorrect behavior of the product 2 - Queries Issues that are related to the query builder or queries in general regression This is behavior that once worked that has broken. Must be resolved before the next release. labels Dec 31, 2024
@melton-jason melton-jason added this to the 7.9.11 milestone Dec 31, 2024
@melton-jason melton-jason self-assigned this Dec 31, 2024
@melton-jason
Copy link
Contributor

melton-jason commented Jan 7, 2025

This largely comes down to how Specify 6 and Specify 7 manages Boolean (True/False) fields in the database.

Specify 6 used a Java library called Hibernate, which handled Booleans by using a BIT(1) column type - a single bit value; thus boolean fields were created with that data type.

While integrating Django within Specify 7, we had to extend the default database engine so that Django can translate the literal bit values to boolean values:

https://github.com/specify/specify7/tree/production/specifyweb/hibernateboolsbackend

django_conversions.update({
FIELD_TYPE.BIT: lambda x: x != b'\x00',
})

For its database purposes, Django uses the bool datatype, which the database vendor interprets as a TINYINT data type:

https://github.com/django/django/blob/40d5516385448a73426aad396778f369a363eda9/django/db/backends/mysql/base.py#L112

https://dev.mysql.com/doc/refman/8.4/en/other-vendor-data-types.html

From a database perspective then, we have two different types of boolean fields. The boolean fields created in Specify 6 use BIT(1), while any new boolean fields created via migrations have the TINYINT datatype.

Django can handle the discrepancy because of the extended engine, but sqlalchemy has no such custom solution: which causes this Issue.

Currently, all boolean fields are being converted to the BIT(1) type for sqlalchemy:

'java.lang.Boolean' : mysql_bit_type}

This Issue arose because of the recent addition of the isStrict field to the SpQueryField table from #5272, but theoretically any Specify 7 boolean field created from migrations could cause this problem.

Whenever a query's fields are accessed to create a report template, MySQL and/or MariaDB have the incorrect column mapping (whenever the value of the boolean field is NULL), which causes the error:

for field in sorted(sp_query.fields, key=lambda field: field.position)

I think a solution to this problem should try and avoid as much technical debt as possible. Having a custom field or attribute on a field (on either the models or datamodel) which assures migrations correctly sets the data type might be an OK solution in the short term, but will cost much more time and mental overhead in the future: we have to remember to always use the solution lest the problem crop up again, and instruct new developers to do the same.

In my opinion, an ideal solution would allow us to define boolean fields as we have and operate in the background (much like the existing custom engine for Django and BIT to Boolean field).

I've been looking into some solutions with using sqlalchemy reflect/inspection tools1 to read and inspect the columns of tables in the database before they are mapped with the sqlalchemy orm.
I'm open for other ideas as well!

Footnotes

  1. autoload_with for table generation, reflection

@realVinayak
Copy link
Contributor

Hmm I suspect the error is because of this line. Could you try opening that in a debugger and see the backtrace? I think it allows you to step inside library functions (python).

https://github.com/sqlalchemy/sqlalchemy/blob/5ad094729b8432cf0b1e08c03caa44f37ab68f88/lib/sqlalchemy/dialects/mysql/types.py#L384-L394

I also suspect that, when using tinyint, value is int (rather than bitstring) thus the iterable error.

Could you, potentially, make a custom type (eh, subclass that BIT class) and override result_processor so that checks if the value is bitstring in the result_processor function (and if not, just returns that value)? The moment 6 is retired, you could make a migration to replace it in the database. Maybe devs should have been using that in the first place though (bit) in sp7.

But, auto inspection and reflection would be cooler. I suspect this issue also breaks the new boolean fields introduced (isPrimary, isSubstrate etc) in query builder. Do those still work?

@acwhite211 acwhite211 linked a pull request Jan 22, 2025 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - Bug Incorrect behavior of the product 2 - Queries Issues that are related to the query builder or queries in general regression This is behavior that once worked that has broken. Must be resolved before the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants