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

Moving columns causes the default uuid() value to get quotes around it #18006

Closed
Nefcanto opened this issue Jan 7, 2023 · 33 comments
Closed
Assignees
Labels
Bug A problem or regression with an existing feature has-pr An issue that has a pull request pending that may fix this issue. The pull request may be incomplete
Milestone

Comments

@Nefcanto
Copy link

Nefcanto commented Jan 7, 2023

Describe the bug

When I set a default value for a column that is UUID to uuid(), and use PHPMyAdmin to move it in the list of columns, and then use show create table X, I see that the uuid() would become 'uuid()' and if I move it one more time, it becomes '''uuid()'''.

To Reproduce

Steps to reproduce the behavior:

  • Create a table with at least 3 columns
  • Set two columns data types to be UUID
  • Set default values for those two columns to uuid()
  • Then use Move column feature to change the order of columns
    • Select the table
    • Go to Structure tab
    • Use Move column link (if you can't find it, search it inside the page)
  • Then use show create table X
  • You should see quotes around the default value
  • If you move columns multiple times, then those quotes will increase in number

Expected behavior

The default value uuid() should remain as is when we move columns (change the order of columns)

Screenshots

From left hand (default state) we get to the right hand after moving columns around

Screenshot from 2023-01-07 16-19-15

Server configuration

  • Operating system:
  • Web server:
  • Database version:
  • PHP version: MariaDB
  • phpMyAdmin version: latest

Client configuration

  • Browser: Chrome
  • Operating system:

Additional context

Add any other context about the bug here.

@williamdes
Copy link
Member

Hi @Nefcanto
Can you try the latest 5.2 version in development (phpMyAdmin 5.2+snapshot) also available as a non official docker image ?

Some UUID changes where made recently: https://github.com/phpmyadmin/phpmyadmin/issues?q=+milestone%3A5.2.1+is%3Aclosed+uuid+

@williamdes williamdes added Bug A problem or regression with an existing feature question Used when we need feedback from the submitter or when the issue is a question about PMA labels Jan 7, 2023
@Nefcanto
Copy link
Author

Nefcanto commented Jan 7, 2023

@williamdes yes sure. Thank you for responding so fast. I'll try with the 5.2 snapshot's non official docker image and I'll report the results here.

@Nefcanto
Copy link
Author

Nefcanto commented Jan 7, 2023

@williamdes I tested using botsudo/phpmyadmin-snapshots:5.3-snapshot and this bug still exists.

This is my docker-compose.yml related to phpMyAdmin:

    pma:
        image: botsudo/phpmyadmin-snapshots:5.3-snapshot
        container_name: FalconPma
        restart: always
        logging:
            driver: none
        ports:
            - 7202:80
        environment: 
            - PMA_HOST=database     
        networks:
            - FalconNetwork

@liviuconcioiu
Copy link
Contributor

In my case, I can't move the columns. I have to remove the single quotes. Looking at this, it seems that uuid is not defined in

https://github.com/phpmyadmin/phpmyadmin/blob/master/libraries/classes/Controllers/Table/Structure/MoveColumnsController.php

and the column can't be moved.

07.01.2023_17.49.13_REC.mp4

@iifawzi
Copy link
Contributor

iifawzi commented Jan 7, 2023

I think, this's most probably related to #17920

@williamdes williamdes added has-pr An issue that has a pull request pending that may fix this issue. The pull request may be incomplete affects/5.2 This issue or pull-request affects 5.2.x releases (and maybe further versions) affects/6.0 This issue or pull-request affects 6.0.x releases (and maybe further versions) confirmed/5.2 This issue is confirmed to be reproduced on 5.2 at the time this label was set confirmed/6.0 This issue is confirmed to be reproduced on 6.0 at the time this label was set and removed question Used when we need feedback from the submitter or when the issue is a question about PMA labels Jan 8, 2023
@williamdes williamdes added this to the 5.2.1 milestone Jan 8, 2023
@Nefcanto
Copy link
Author

Nefcanto commented Jan 8, 2023

@liviuconcioiu does your pull request solve this? Can I test it?

@williamdes
Copy link
Member

@liviuconcioiu does your pull request solve this? Can I test it?

I think it should, you can apply the patch from the pull-request and let us know

@Nefcanto
Copy link
Author

Nefcanto commented Jan 8, 2023

@williamdes I don't know how to apply the patch from the pull-request. Can I use it as a docker image?

@williamdes
Copy link
Member

I could build this some time soon
Or repair @sudo-robot for it to deploy the fix

@liviuconcioiu
Copy link
Contributor

liviuconcioiu commented Jan 8, 2023

@Nefcanto the PR solves the problem. No more quotes and it allows the columns to be moved.

08.01.2023_11.57.09_REC.mp4

To test it, is very easy. Just go to https://github.com/phpmyadmin/phpmyadmin/pull/18007/files and copy the code from the right side and paste it at the specified line numbers on this file:

libraries/classes/Controllers/Table/Structure/MoveColumnsController.php

@Nefcanto
Copy link
Author

Nefcanto commented Jan 8, 2023

@williamdes and @liviuconcioiu you are awesome guys. Thank you so much for the time you spent on this.

@williamdes williamdes self-assigned this Jan 8, 2023
williamdes added a commit that referenced this issue Jan 8, 2023
Pull-request: #18007

Signed-off-by: William Desportes <williamdes@wdes.fr>
@williamdes
Copy link
Member

@williamdes and @liviuconcioiu you are awesome guys. Thank you so much for the time you spent on this.

The fix is now into the snapshot and docker snapshot image, you can pull it.

@Nefcanto

This comment was marked as outdated.

@williamdes

This comment was marked as resolved.

@Nefcanto

This comment was marked as resolved.

@williamdes

This comment was marked as outdated.

@Nefcanto
Copy link
Author

Nefcanto commented Jan 9, 2023

@williamdes thank you

@williamdes

This comment was marked as resolved.

@williamdes williamdes removed affects/5.2 This issue or pull-request affects 5.2.x releases (and maybe further versions) affects/6.0 This issue or pull-request affects 6.0.x releases (and maybe further versions) confirmed/5.2 This issue is confirmed to be reproduced on 5.2 at the time this label was set confirmed/6.0 This issue is confirmed to be reproduced on 6.0 at the time this label was set labels Jan 10, 2023
@Nefcanto

This comment was marked as resolved.

@williamdes

This comment was marked as outdated.

@Nefcanto

This comment was marked as resolved.

@williamdes
Copy link
Member

@williamdes it's not related to the docker pull. The pull works just fine. The bug still exists. Would you like to see it in action? Should I send a Google Meet?

Indeed, sorry for this. I tried the image when I was on my workstation.

I can move columns without any issue. I tested the same as you with an uuid() as default. And other variants.
Can you paste the "preview sql" query?

I do not think a google meet is best for such a small bug :)
Note: on my homepage the version is 5.2.1-dev+20230109.4b5c494aee

@Nefcanto
Copy link
Author

Nefcanto commented Jan 11, 2023

@williamdes , it's nice of you that you put time into this, there is no need for excuses. I'm the one who should be grateful here. Thank you so much.

Please create a table using this script:

CREATE TABLE `Temp` (
  `Id` int(11) NOT NULL,
  `First` text NOT NULL DEFAULT uuid(),
  `Second` text NOT NULL DEFAULT uuid()
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci

Then move the second column between the Id and First. The using show create table Temp, do you see this?

CREATE TABLE `Temp` (
  `Id` int(11) NOT NULL,
  `Second` text NOT NULL DEFAULT 'uuid()',
  `First` text NOT NULL DEFAULT uuid()
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci

Then please move the Id to the last column. Do you see this?

CREATE TABLE `Temp` (
  `Second` text NOT NULL DEFAULT '\'uuid()\'',
  `First` text NOT NULL DEFAULT 'uuid()',
  `Id` int(11) NOT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci

I'm sorry, I don't know PHP and I don't know I should put what file where. :( I just use MariaDB with .NET API and React client.

Alos, this is my home page info:

Screenshot from 2023-01-11 09-29-54

@Nefcanto
Copy link
Author

Nefcanto commented Jan 11, 2023

Also I realized that I can change the column's nullability and it fixes the 'uuid()' back to uuid(). Or just hit the Change link for the column and save without any changes. It fixes the problem.

@williamdes
Copy link
Member

Thank you for the feedback, I will try using MariaDB 10.10

There is maybe a difference
I had tried on 10.8

@williamdes williamdes reopened this Jan 11, 2023
@liviuconcioiu
Copy link
Contributor

liviuconcioiu commented Jan 11, 2023

@williamdes I've attached a video, with @Nefcanto queries. This happens because @Nefcanto uses TEXT as datatype instead of UUID.

11.01.2023_13.49.58_REC.mp4

Technically this isn't a bug, because the value he sees, 'uuid()' is the default value for TEXT.

11.01.2023_14.03.45_REC.mp4

@Nefcanto
Copy link
Author

@liviuconcioiu thank you. I have to use text data types because I store slug. But if the slug is empty for a given post, then I should create a unique token there, and there is no better unique token than UUID. That's why I use it as the default value for slugs and keys.

@liviuconcioiu
Copy link
Contributor

@liviuconcioiu thank you. I have to use text data types because I store slug. But if the slug is empty for a given post, then I should create a unique token there, and there is no better unique token than UUID. That's why I use it as the default value for slugs and keys.

For the the column where a uuid is stored, you can change it to UUID.

uuid

@williamdes
Copy link
Member

williamdes commented Jan 12, 2023

So it seems its a bug from the videos I see, the function is moved to an escaped string
It looks like it is the bug #14371

@williamdes
Copy link
Member

Could someone contribute to fixing this left over please ? 🙏🏻

As I said on #17830 (comment) the release should occur at the end of the month.
Testers and translators are welcome !

@Nefcanto
Copy link
Author

@williamdes I tested with the botsudo/phpmyadmin-snapshots:5.2-snapshot and this bug is not solved. Each time I move a string column that has a uuid() function as its default value, that uuid() function gets a pair of single quotes around it. If I move it once, it becomes 'uuid()'. If I move it twice, it becomes '''uuid'''.

This problem does not exist for uuid column. But it shows itself for string columns which have uuid() function as their default value.

@williamdes
Copy link
Member

@williamdes I tested with the botsudo/phpmyadmin-snapshots:5.2-snapshot and this bug is not solved. Each time I move a string column that has a uuid() function as its default value, that uuid() function gets a pair of single quotes around it. If I move it once, it becomes 'uuid()'. If I move it twice, it becomes '''uuid'''.

Okay, got it

This problem does not exist for uuid column.

Awesome

But it shows itself for string columns which have uuid() function as their default value.

Because of that it's maybe more #14371 like I said on #18006 (comment)

@Nefcanto
Copy link
Author

Yep, you're right, it's similar to the #14371.

@ibennetch ibennetch modified the milestones: 5.2.1, 5.2.2 Feb 8, 2023
williamdes added a commit that referenced this issue Apr 28, 2024
Signed-off-by: William Desportes <williamdes@wdes.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A problem or regression with an existing feature has-pr An issue that has a pull request pending that may fix this issue. The pull request may be incomplete
Projects
None yet
Development

No branches or pull requests

5 participants