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

fix: [Postgre] updateBatch() breaks char type data #8524

Merged

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Feb 7, 2024

Description
Follow-up #8439
From https://forum.codeigniter.com/showthread.php?tid=89336

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer labels Feb 7, 2024
Copy link
Member

@sclubricants sclubricants left a comment

Choose a reason for hiding this comment

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

This looks good.

One other option - though I don' t know that it would be any better would be to change CHARACTER into CHARACTER VARYING in the cast method. The only reason is that it would keep the getFieldType() method more 'pure' in a sense. But I'm not sure this would be any better.

@sclubricants
Copy link
Member

sclubricants commented Feb 9, 2024

You could change to CHARACTER | CHAR TO VARCHAR in the cast() method. This should cover the other DBMS too.

https://onecompiler.com/postgresql/423ynm2e2

CREATE TABLE test_table
(
    id      integer,
    char    char(10),
    varchar varchar(10)
);

INSERT INTO test_table (id, char, varchar) VALUES (1, 'foo', 'foo');
INSERT INTO test_table (id, char, varchar) VALUES (2, 'bar', 'bar');
INSERT INTO test_table (id, char, varchar) VALUES (3, 'baz', 'baz');

UPDATE "test_table"
SET "char"    = CAST(_u."char" AS VARCHAR),
    "varchar" = CAST(_u."varchar" AS CHARACTER VARYING)
FROM (SELECT 'Foo' "char", 1 "id", 'Foo' "varchar"
      UNION ALL
      SELECT 'Bar' "char", 2 "id", 'Bar' "varchar"
      UNION ALL
      SELECT 'Baz' "char", 3 "id", 'Baz' "varchar") _u
WHERE "test_table"."id" = CAST(_u."id" AS INTEGER);

SELECT * FROM test_table;
    private function cast($expression, ?string $type): string
    {
        if (strtoupper($type) === 'CHAR' || strtoupper($type) === 'CHARACTER') {
            $type = 'VARCHAR';
        }

        return ($type === null) ? $expression : 'CAST(' . $expression . ' AS ' . strtoupper($type) . ')';
    }

@kenjis
Copy link
Member Author

kenjis commented Feb 10, 2024

@sclubricants Thank you for the review!

If character (or char) lacks a specifier, it is equivalent to character(1).
So on PostgreSQL, character(n) seems to be correct type.
And it does not seem to be pure to change CHAR to VARCHAR, too.

Anyway, the method is only for Postgre, and it will override even if the parent class has the method.
So I think there is no problem. Go with this for now.

@kenjis kenjis merged commit 93e281a into codeigniter4:develop Feb 10, 2024
62 checks passed
@kenjis kenjis deleted the fix-postgre-updateBatch-character branch February 10, 2024 00:16
@sclubricants
Copy link
Member

CHAR = CHARACTER
VARCHAR = CHARACTER VARYING
VARCHAR = CHAR of unspecified length

Anyways, looks good

@sclubricants
Copy link
Member

SELECT CAST('STRING' AS VARCHAR) AS TEST;

This runs on postgre but not on mysql

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants