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

Field description does not appear #637

Closed
lukinhaspm opened this issue May 4, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@lukinhaspm
Copy link

commented May 4, 2019

Steps to reproduce this issue

  1. Step 1;
    Create the table
CREATE TABLE `teste` (
  `aip_id` int(11) unsigned NOT NULL AUTO_INCREMENT COMMENT 'Id',
  `aip_por` smallint(5) unsigned NOT NULL COMMENT 'por',
  `aip_em` datetime NOT NULL DEFAULT current_timestamp() COMMENT 'em ??',
  `aip_por2` smallint(5) unsigned DEFAULT NULL COMMENT 'por',
  `aip_em2` datetime DEFAULT NULL ON UPDATE current_timestamp() COMMENT 'em',
  PRIMARY KEY (`aip_id`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1 ROW_FORMAT=DYNAMIC COMMENT='Teste'
  1. Step 2;

Look into the fields grid

image

  1. Step 3;

Show create table teste

Current behavior

Field description does not appear

I saw this point when I use the On Update default value

Expected behavior

Possible solution

Environment

  • HeidiSQL version:
    10.1.0.5552
  • Database system and version:
    MariaDB 10.2.x
  • Operating system:
    Windows and Linux
@rentalhost

This comment has been minimized.

Copy link
Collaborator

commented May 4, 2019

What do you expects here?

@lukinhaspm

This comment has been minimized.

Copy link
Author

commented May 5, 2019

I expect the coment "em"
image

all columns has comments, but heidi not show all when we have "On update" in default value

@rentalhost

This comment has been minimized.

Copy link
Collaborator

commented May 5, 2019

Oh, now I got and I could reproduce it!

Simplified case:

DROP TABLE IF EXISTS test;

CREATE TABLE test (
  `column` DATETIME ON UPDATE current_timestamp() COMMENT 'Comment'
);

Inspection:

The SHOW CREATE TABLE test will report it correctly:

CREATE TABLE `test` (
  `column` datetime DEFAULT NULL ON UPDATE current_timestamp() COMMENT 'Comment'
  ############################################################ ^^^^^^^^^^^^^^^^^
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4

Seems to be related to:

// Do the same for a potentially existing ON UPDATE clause
rxCol.Expression := '^\s*ON UPDATE\s+(.*)($|\s+(COLUMN_FORMAT|COMMENT|INVISIBLE)\b)';
if rxCol.Exec(ColSpec) then begin
Col.OnUpdateText := Trim(rxCol.Match[1]);
Col.OnUpdateText := Col.OnUpdateText.TrimRight([',']);
Col.OnUpdateType := cdtExpression;
Delete(ColSpec, 1, rxCol.MatchLen[1]);
end;

The regular expression above will matches the COMMENT part when it have the ON UPDATE clause, instead of read only the content related to this second part. So the following real COMMENT parse part is ignored because the COMMENT was consumed here.

// Comment
Col.Comment := ExtractLiteral(ColSpec, 'COMMENT');

Possible fix:

  • Replace from: ^\s*ON UPDATE\s+(.*)($|\s+(COLUMN_FORMAT|COMMENT|INVISIBLE)\b) (example)
  • Replace to: ^\s*ON UPDATE\s+(.*?)(?:$|\s+(?=COLUMN_FORMAT|COMMENT|INVISIBLE)\b) (example)

Note: don't copy directly from regex101 because I have to drop the initial ^, that exists on original regular expression to matches the start of the string (that was consumed by previous procedures).

It must:

  • From (.*) to (.*?) make it lazy (so will stop if the next match pattern could be applied, in this case, the COMMENT;
  • From ( to (?: will ignore the second match, that is not need here;
  • From ( to (?= will do a positive lookahead by COMMENT to allow "stop" the matching when it is applied;

Additional notes:

If you are modifying the table it will not lost the comment after save, except if you are modifying specifically a column that have ON UPDATE:

ALTER TABLE `test`
	ALTER `column` DROP DEFAULT;
ALTER TABLE `test`
	CHANGE COLUMN `column` `column` DATETIME NOT NULL FIRST;

Nodev note:

I just don't know if Pascal allows that, but basically, only the first and third change is need.

@ansgarbecker

This comment has been minimized.

Copy link
Collaborator

commented May 5, 2019

I had to apply a different approach as TSynRegExpr does not seem to support positive lookahead. However, I could reproduce the issue before, and after applying my fix I can't.

@ansgarbecker ansgarbecker added this to the v10.2 milestone May 5, 2019

ansgarbecker added a commit that referenced this issue May 6, 2019

Issue #637: No need to upper-case when calling StartsWith() with igno…
…re case flag. Also, prefer StartsWith() over various ugly calls to UpperCase(Copy()).
@lukinhaspm

This comment has been minimized.

Copy link
Author

commented May 6, 2019

Thanks @ansgarbecker works very fine!

@ansgarbecker

This comment has been minimized.

Copy link
Collaborator

commented May 6, 2019

No problem.
@rentalhost thanks for your exact findings!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.