Adding an autoincrement field with Firebird 3.0

[manolopygithub]

Since Firebird 3.0 it allows define autoincremented field, the sufix to create such fiields is:

"GENERATED BY DEFAULT AS IDENTITY NOT NULL".

Therefore, it is necesary define a variable to do that, and then ask if that variable is True to add that sufix to the definition of the field.

_genFields.txt

[dregad]

Hello,

As a preliminary remark, instead of attaching a text file with modified code, it would be a lot better to submit a pull request with the patch. It would make reviewing and collaboration as well as eventual merging of changes a lot easier.

Anyway I had a quick look at the modified function in your text file, and unfortunately your approach is not correct and cannot be applied as it is, because database-specific code does not belong in the parent class function ADODB_DataDict::_genFields().

The right way to do this is to adapt the driver-specific data dictionary class, in this case the ADODB2_firebird::_createSuffix() method, where you can leverage the existing $fautoinc parameter to have the function return the correct suffix for autoincrement columns.

One more remark on your patch:

$lines[$fid] = $fname.' '.$ftype.' GENERATED BY DEFAULT AS IDENTITY NOT NULL';

I'm not very familiar with Firebird so maybe this is not an issue, but the documentation states that Identity columns are implicitly NOT NULL. Consequently, it feels unnecessary and possibly wrong to add NOT NULL in the suffix as you did.

[manolopygithub]

Good afternoon Damien, Thank you very much for your reply, I really appreciate it and I apologize for the delay in responding to you, I was very busy the past two weeks. I will follow all your suggestions about the pull request and the code structure. About the "NOT NULL" I will check it out again. Best Regards!

…

On Thu, Mar 21, 2024 at 1:34 PM Damien Regad ***@***.***> wrote: Hello, As a preliminary remark, instead of attaching a text file with modified code, it would be a lot better to submit a pull request <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request> with the patch. It would make reviewing and collaboration as well as eventual merging of changes a lot easier. Anyway I had a quick look at the modified function in your text file, and unfortunately your approach is not correct and cannot be applied as it is, because database-specific code does not belong in the parent class function ADODB_DataDict::_genFields(). The right way to do this is to adapt the driver-specific data dictionary class, in this case the ADODB2_firebird::_createSuffix() method, where you can leverage the existing *$fautoinc* parameter to have the function return the correct suffix for autoincrement columns. One more remark on your patch: $lines[$fid] = $fname.' '.$ftype.' GENERATED BY DEFAULT AS IDENTITY NOT NULL'; I'm not very familiar with Firebird so maybe this is not an issue, but the documentation <https://firebirdsql.org/file/documentation/chunk/en/refdocs/fblangref30/fblangref30-ddl-table.html#fblangref30-ddl-tbl-identity> states that *Identity columns are implicitly NOT NULL*. Consequently, it feels unnecessary and possibly wrong to add NOT NULL in the suffix as you did. — Reply to this email directly, view it on GitHub <#1024 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AX3I32I2ZSZRMKO3JLFXZEDYZMDZ3AVCNFSM6AAAAABEW3EAJKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJSHEYTQNJQG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>