Hey, thanks for the PR !

Overall LGTM, but it's missing a test case for passing an array of fields into .onConflict.

I could add those if you'd like 😄

This also needs documentation - could you create a PR in https://github.com/knex/documentation?

@nicoburns tell me if you want help on the missing test case / the documentation, I'd be glad to help

I've put in a documentation PR here: knex/documentation#260.

Feedback on the documentation would definitely be helpful. I found it a little tricky to balance being clear and describing all possible combinations of options with keeping the explanation concise. Ideally there should probably be a separate section on upserting with a guide-level explanation that explains things like setting of the schema to make it work (similar to the section on transactions). But thats a much bigger change to the structure of the docs, and could always come later.

Help implementing extra tests would also be helpful. In addition to @SherloxTV's suggestion of a test that covers passing an array of fields into .onConflict, an integration test that covers use of merge with an array of inserted objects if probably a good idea.

Edit: these have now been added. I think the main outstanding task is updating the TypeScript definitions.

I'm using knex for a project that can use either sqlite, mssql or oracle. For the time being this project would let me use onConflict() with only sqlite. My question is, what would a program look like that would like to use the onConflict() syntax when supported by the current dialect. Specifically what goes in the if-clause in place of hasOnConflict() here?

if (hasOnConflict()) {
    // Use onConfict to upsert
} else {
    // use delete followed by insert
}

@nicoburns Do you need any help finishing this?

as someone who woud love to see this merged, what are the meaning of the two labels here? One says missing documentation, but there is a PR for it here knex/documentation#260. The other one says missing tests, but there are a few hundred lines of testing code here. What cases specifically would you like to see tested? I do see some merge conflicts against master that need resolving though.

What cases specifically would you like to see tested? I do see some merge conflicts against master that need resolving though.

Integration tests, which verifies that when those queries are ran against real database they behave as expected. In other words all this: #3186 (comment)

Mostly the same queries, whose generation is currently covered by query builder code generation tests.

Amazing @nicoburns thanks! Looks like it's super close. The ramining failing tests I'm seeing are:

  1) Integration Tests
       oracle | oracledb
         Inserts
           will silently do nothing when multiple inserts are made into a unique column and ignore is specified:
     Error: insert into "upsert_tests" ("email", "name") values (:1, :2) returning "email" into :3 - ORA-00001: unique constraint (SYSTEM.upsert_tests_email_unique) violated
  
  2) Integration Tests
       oracle | oracledb
         Inserts
           will silently do nothing when multiple inserts are made into a composite unique column and ignore is specified:
     Error: insert into "upsert_composite_key_tests" ("email", "name", "org") values (:1, :2, :3) returning "email" into :4 - ORA-00001: unique constraint (SYSTEM.pWLdp/gASIHRY5hXRVs73MBLb1E) violated
  
  3) Integration Tests
       mssql | mssql
         Inserts
           will silently do nothing when multiple inserts are made into a unique column and ignore is specified:
     RequestError: insert into [upsert_tests] ([email], [name]) output inserted.[email] values (@p0, @p1) - Cannot insert duplicate key row in object 'dbo.upsert_tests' with unique index 'upsert_tests_email_unique'. The duplicate key value is (ignoretest@example.com).

  4) Integration Tests
       mssql | mssql
         Inserts
           will silently do nothing when multiple inserts are made into a composite unique column and ignore is specified:
     RequestError: insert into [upsert_composite_key_tests] ([email], [name], [org]) output inserted.[email] values (@p0, @p1, @p2) - Cannot insert duplicate key row in object 'dbo.upsert_composite_key_tests' with unique index 'upsert_composite_key_tests_org_email_unique'. The duplicate key value is (acme-inc, ignoretest@example.com).

So it's specific to mssql and oracle. If it were an issue remaining with Postgres I'd hop on it since I'm an expert in Postgres, but I've worked with mssql and oracle zero in my career so I don't feel comfortable with such an important diff on a DB engine I'm not familiar with.

Anyone from the community who's expressed interest able to hop on and fix these last failures? cc @vazra @ARKKYN @a-tokyo or others?

I've pushed a commit ignoring those tests for oracle and mssql, as those platforms are not currently supported by this feature. Perhaps we ought to have some kind of runtime check that throws an error if someone tries to use this feature with those databases?

For those following this PR, there is also still ongoing work on the corresponding documentation taking place here: knex/documentation#260

Perhaps we ought to have some kind of runtime check that throws an error if someone tries to use this feature with those databases?

Sounds good!

Perhaps we ought to have some kind of runtime check that throws an error if someone tries to use this feature with those databases?

This is now implemented.

Additionally I have created a new "class" (ES5 syntax) OnConflictQueryBuilder which is returned from .onConflict() and wraps the regular query builder. It has two methods: .ignore() and .merge() which modify the main query builder and return it back to you. The end result of this is that the interface for onConflict queries is the same for valid queries, but invalid combinations of .onConflict(), .ignore(), and .merge() are impossible. I also added a .then() method to OnConflictQueryBuilder which throws, to catch the case where someone tries to await an query that ends in a .onConflict() (which is invalid without a matching .ignore() or .merge().

🎉 Amazing work @nicoburns, definitely want this feature to Knex, @elhigu anything else to add to this PR? 😄

Thanks for all the hard work on this @nicoburns 👏

@kibertoad how long would you expect for it to take to get this master-merged commit into the next official release version?

Will be released as soon as documentation is finalized by @nicoburns

Released in 0.21.10

Thank you so much to the knex contributors for implementing this wonderful feature. Saves a lot of boilerplate code on my side. Congrats on the API too.

@kibertoad Someone implemented it for mssql here: #6050. I tested it and it seems to work well at least on my local environment

UPDATE: except for unique composite indexes and tables with triggers

It's not documented? https://knexjs.org/guide/query-builder.html#upsert Or can't we use the simple upsert method for Postgres? That would be a shame.

@melroyvandenberg In PG you use .onConflict().merge() https://knexjs.org/guide/query-builder.html#onconflict