Bug 30060 - Missing primary key on user_permissions
Summary: Missing primary key on user_permissions
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 30912 29125 30055 30276
  Show dependency treegraph
 
Reported: 2022-02-09 12:52 UTC by Jonathan Druart
Modified: 2022-06-09 08:20 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 30060: Add primary key to user_permissions table (2.54 KB, patch)
2022-02-09 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30060: Add primary key to user_permissions table (2.63 KB, patch)
2022-02-09 13:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30060: Add primary key to user_permissions table (3.29 KB, patch)
2022-02-13 12:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30060: Add primary key to user_permissions table (3.35 KB, patch)
2022-02-15 12:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30060: (follow-up) Remove duplicate prior to adding key (1.76 KB, patch)
2022-02-15 12:19 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-02-09 12:52:03 UTC

    
Comment 1 Jonathan Druart 2022-02-09 12:55:28 UTC
Without primary key DBIC is failing with something like:

GET /api/v1/suggestions/managers: unhandled exception (DBIx::Class::Exception)<<DBIx::Class::ResultSource::RowParser::_resolve_collapse(): Unable to calculate a definitive collapse co
lumn set for UserPermission (last member of the Borrower -> user_permissions chain): fetch more unique non-nullable columns at /kohadevbox/koha/Koha/Objects.pm line 394>> Koha::REST::Plugin::Exceptions::__ANON__ /
kohadevbox/koha/Koha/REST/Plugin/Exceptions.pm (73)
Comment 2 Jonathan Druart 2022-02-09 12:55:58 UTC
Created attachment 130356 [details] [review]
Bug 30060: Add primary key to user_permissions table
Comment 3 Jonathan Druart 2022-02-09 12:58:48 UTC
Test plan:
Execute the atomic update and confirm that the new PK has been added to the user_permissions table.
Comment 4 Tomás Cohen Arazi 2022-02-09 13:19:17 UTC
Created attachment 130360 [details] [review]
Bug 30060: Add primary key to user_permissions table

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Katrin Fischer 2022-02-13 12:53:06 UTC
Created attachment 130533 [details] [review]
Bug 30060: Add primary key to user_permissions table

Without primary key DBIC is failing with something like:

GET /api/v1/suggestions/managers: unhandled exception (DBIx::Class::Exception)<<DBIx::Class::ResultSource::RowParser::_resolve_collapse(): Unable to calculate a definitive collapse co
lumn set for UserPermission (last member of the Borrower -> user_permissions chain): fetch more unique non-nullable columns at /kohadevbox/koha/Koha/Objects.pm line 394>> Koha::REST::Plugin::Exceptions::__ANON__ /
kohadevbox/koha/Koha/REST/Plugin/Exceptions.pm (73)

Test plan:
Execute the atomic update and confirm that the new PK has been added to the user_permissions table.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 6 Katrin Fischer 2022-02-13 12:54:15 UTC
Please remember guidelines for commit messages :)

Do we need to worry about potentially doubled up entries? 

I recently managed to do that with SQL manipulations, but haven't seen it happen from within Koha
Comment 7 Martin Renvoize 2022-02-15 12:19:35 UTC
Created attachment 130616 [details] [review]
Bug 30060: Add primary key to user_permissions table

Without primary key DBIC is failing with something like:

GET /api/v1/suggestions/managers: unhandled exception (DBIx::Class::Exception)<<DBIx::Class::ResultSource::RowParser::_resolve_collapse(): Unable to calculate a definitive collapse co
lumn set for UserPermission (last member of the Borrower -> user_permissions chain): fetch more unique non-nullable columns at /kohadevbox/koha/Koha/Objects.pm line 394>> Koha::REST::Plugin::Exceptions::__ANON__ /
kohadevbox/koha/Koha/REST/Plugin/Exceptions.pm (73)

Test plan:
Execute the atomic update and confirm that the new PK has been added to the user_permissions table.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2022-02-15 12:19:39 UTC
Created attachment 130617 [details] [review]
Bug 30060: (follow-up) Remove duplicate prior to adding key

I had issues with this update as somehow my user_permissions table
contained duplicate rows which throws an error when adding the new
primary key.

This follow-up patch adds SQL to remove any duplicate rows prior to
adding the new primary key.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Jonathan Druart 2022-02-15 12:30:08 UTC
Thanks for the follow-up, Martin. However I don't think the "temp" column is needed. See installer/data/mysql/db_revs/210600012.pl where we delete duplicate from language tables.
Comment 10 Martin Renvoize 2022-02-16 13:57:09 UTC
(In reply to Jonathan Druart from comment #9)
> Thanks for the follow-up, Martin. However I don't think the "temp" column is
> needed. See installer/data/mysql/db_revs/210600012.pl where we delete
> duplicate from language tables.

Hmm, the difference here is that there is no incrementing key field in the table to start with.. I couldn't work out how to do it without creating the temporary field. More than happy to be corrected, but the approach in update you point to matches the approach I have here.. it relies on an incrementing key field which we don't have here until I add it temporarily.
Comment 11 Jonathan Druart 2022-02-21 09:48:00 UTC
(In reply to Martin Renvoize from comment #10)
> (In reply to Jonathan Druart from comment #9)
> > Thanks for the follow-up, Martin. However I don't think the "temp" column is
> > needed. See installer/data/mysql/db_revs/210600012.pl where we delete
> > duplicate from language tables.
> 
> Hmm, the difference here is that there is no incrementing key field in the
> table to start with.. I couldn't work out how to do it without creating the
> temporary field. More than happy to be corrected, but the approach in update
> you point to matches the approach I have here.. it relies on an incrementing
> key field which we don't have here until I add it temporarily.

Indeed!
Comment 12 Fridolin Somers 2022-02-25 00:36:45 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 13 Magnus Enger 2022-03-11 08:44:19 UTC
This db update is giving me an error when I try to upgrade a gitified install to current master. Reported as bug 30276.
Comment 14 Jonathan Druart 2022-03-21 10:58:36 UTC
DBrev does not work, please have a look at bug 30276.
Comment 15 Marcel de Rooy 2022-06-09 06:10:29 UTC
From ML:

 - Upgrade to 21.12.00.016 [04:05:32]: Bug 30060 - Update
   user_permissions to add primary key and remove null option from code column
   ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: You have an
   error in your SQL syntax; check the manual that corresponds to your MySQL
   server version for the right syntax to use near 'IF EXISTS `PRIMARY`' at
   line 1 at /usr/share/koha/lib/C4/Installer.pm line 739
Comment 16 Jonathan Druart 2022-06-09 06:43:50 UTC
(In reply to Marcel de Rooy from comment #15)
> From ML:
> 
>  - Upgrade to 21.12.00.016 [04:05:32]: Bug 30060 - Update
>    user_permissions to add primary key and remove null option from code
> column
>    ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: You have an
>    error in your SQL syntax; check the manual that corresponds to your MySQL
>    server version for the right syntax to use near 'IF EXISTS `PRIMARY`' at
>    line 1 at /usr/share/koha/lib/C4/Installer.pm line 739

See bug 30912.