Skip to content

Fix boolean fields represented as integers in database schema#2689

Merged
demiankatz merged 6 commits into
vufind-org:devfrom
skellamp:User_list-Upgrade
Feb 10, 2023
Merged

Fix boolean fields represented as integers in database schema#2689
demiankatz merged 6 commits into
vufind-org:devfrom
skellamp:User_list-Upgrade

Conversation

@skellamp
Copy link
Copy Markdown
Contributor

@skellamp skellamp commented Jan 31, 2023

Changed the data type of public column in user_list to tinyint.

TODO

  • Apply similar fixes to the saved column in the searches table
  • Run full test suite with MySQL
  • Test upgrade tool with MySQL existing data
  • Apply equivalent changes to PostgreSQL schema
  • Add migration script for PostgreSQL
  • Run full test suite with PostgreSQL
  • Test migration script in PostgreSQL with existing data
  • Incorporate changes from this PR into Convert from Laminas\Db to Doctrine #2233 after merging.

Changed the data type of public column in user_list to tinyint.
Copy link
Copy Markdown
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @skellamp -- I think we also need to change int to boolean for the equivalent field in pgsql.sql, and we'll also need to add a migration script for PostgreSQL users. I'll put TODO checkboxes in place about all of this -- we should probably test that the change is safe in MySQL before we spend time working on PostgreSQL as well.

Comment thread module/VuFind/sql/mysql.sql Outdated
@demiankatz demiankatz changed the title User_list table update Fix boolean fields represented as integers in database schema Feb 7, 2023
Copy link
Copy Markdown
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skellamp, I tried to do a test of upgrading existing data in PostgreSQL using the migration script, but it doesn't seem to be working. Here's what I did:

1.) I started up VuFind with the -Ddbtype=pgsql switch while on the dev branch
2.) I created an account in the instance, created public and private lists, created some search history, and saved a search. This gave me data where I could check that flags were being properly applied after the migration.
3.) I switched to your branch
4.) I ran the migration with psql -U vufindtest -d vufind_test -a -f module/VuFind/sql/migrations/pgsql/9.0/006-modify-boolean-fields.sql

Here's what I got (after entering the default test environment password of "vufindpass"):

--
-- Modifications to table `search`
--
ALTER TABLE "search"
  ALTER COLUMN saved TYPE boolean NOT NULL DEFAULT '0';
psql:module/VuFind/sql/migrations/pgsql/9.0/006-modify-boolean-fields.sql:6: ERROR:  syntax error at or near "NOT"
LINE 2:   ALTER COLUMN saved TYPE boolean NOT NULL DEFAULT '0';
                                          ^
--
-- Modifications to table `user_list`
--
ALTER TABLE "user_list"
  ALTER COLUMN public TYPE boolean NOT NULL DEFAULT '0';
psql:module/VuFind/sql/migrations/pgsql/9.0/006-modify-boolean-fields.sql:13: ERROR:  syntax error at or near "NOT"
LINE 2:   ALTER COLUMN public TYPE boolean NOT NULL DEFAULT '0';

I think PostgreSQL syntax requires you to have a separate clause for each thing you change in a row. See, for example, https://github.com/vufind-org/vufind/blob/dev/module/VuFind/sql/migrations/pgsql/3.0/004-modify-seach-columns.sql for something similar.

However, when I tried to adjust the syntax to this style, I still ran into errors about type-casting of data and defaults, so I think a little more work/research is needed. Can you investigate further? Let me know if you need any more help from my end!

Copy link
Copy Markdown
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @skellamp, this looks good and everything is working as expected for me in MySQL and PostgreSQL now.

@demiankatz demiankatz merged commit 08a28ae into vufind-org:dev Feb 10, 2023
EreMaijala pushed a commit to EreMaijala/vufind that referenced this pull request Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants