Is returnValues a necessary functionality of a dialect

[dazhuangc]

Hi,

I am developing a new dialect SAP HANA. HANA does not natively support returnValues. And I notice that only mssql and postgres are configured as supporting returnValues, other dialects use default returnValues: false in dialects/abstract/index.ts.
So I guess the returnValues may not be necessary.

But when running integration tests, I find that in the query-generator.js#insertQuery(), the options.returning is set to true. This makes me think that returnValues is a must-have functionality.

I am a bit confused. Could you please confirm if it is necessary?

[ephys]

Supporting returnValues is not necessary and indeed only a couple of dialects support it

The current implementation of insertQuery will ignore the returning option if the supports.returnValues config of the dialect is false

[dazhuangc]

Thank you @ephys for the reply.

Then I have a further question:
I got an error 'Cannot set properties of undefined (setting 'isNewRecord')' in the dialects/abstract/query-interface.js#insert() when running integration tests,

async insert(instance, tableName, values, options) {
    // ...
    const results = await this.sequelize.queryRaw(query, options);
    if (instance) {
      results[0].isNewRecord = false;  // error here, results[0] is undefined
    }

    return results;
}

When implement the query-generator.js#insertQuery() simply by INSERT INTO "users" ("id","createdAt","updatedAt") VALUES (?,?,?), I think it makes sense to return no results (results[0] is undefined), because returnValues is not supported in this case. But the undefined error happens.

Do you have any suggestion to solve this error? Should I rewrite the insert() function in dialects/hana/query.js (I am not sure if this is proper, because no other dialects rewrite dialects/abstract/query-interface.js#insert())?

[ephys]

I think all dialects are currently able to return the primary key of the field that was inserted when it's an auto-incremented one. Is that something that HANA can do?

[dazhuangc]

Yes, HANA can return the last generated ID by a do-begin-end block.

DO (IN id INT => ?, IN createdAt TIMESTAMP=> ?, IN updatedAt TIMESTAMP=> ?)
BEGIN
  INSERT INTO "users" ("id","createdAt","updatedAt") VALUES (:id,:createdAt,:updatedAt);
  SELECT CURRENT_IDENTITY_VALUE() FROM DUMMY;
END;

The block is needed because HANA insert statement does not return anything.

I was thinking that once the returnValues is set to false, the insert statement should return nothing.

If I understand correctly now, even if returnValues is false, the insert statement still need to return a row, which contains the generated primary key or ID, is this correct?

[ephys]

Yep! We always return the auto-incremented primary key as otherwise the instance would not be able to be used for further queries

[dazhuangc]

Thank you Alyx!

[ephys]

In case you were working on an implementation with the goal of opening a PR to Sequelize, I want to make sure to avoid bad surprises so I took a minute to document our current process to access new dialects on our website

It's still a draft, but it should accurately represent our current process - https://6613cff1f540a100a20dbfd3--sequelize-site.netlify.app/docs/v7/databases/new/ (PR sequelize/website#742)

[dazhuangc]

Thank! This information helps a lot. I will refer to these PRs and documents.

I am still working in packages/core/src/dialects/hana/. Should I now move the new HANA dialect under development to @sequelize/hana(packages/hana)?

[ephys]

Yep!