directus icon indicating copy to clipboard operation
directus copied to clipboard

Allow manual setting auto incremented field value on create

Open u12206050 opened this issue 2 years ago β€’ 9 comments

Description

Allow to manually set auto incremented field if it is requried, might still be disabled

Only allows adding a value if the field itself is set to required: Require value to be set on creation part1

Disabling the field on create is still possible. part2

Fixes #14652

Type of Change

  • [x] Bugfix
  • [x] Improvement
  • [ ] New Feature
  • [ ] Refactor / codestyle updates
  • [ ] Other, please describe:

Requirements Checklist

  • [ ] New / updated tests are included
  • [ ] All tests are passing locally
  • [ ] Performed a self-review of the submitted code

If adding a new feature:

  • [ ] Documentation was added/updated

u12206050 avatar Jul 26 '22 13:07 u12206050

We shouldn't be mixing auto increments with manually entered IDs (see #14652). Adding support for manually entered IDs that aren't auto-incremented (like the "manual string" type primary key) is something we can discuss πŸ‘πŸ»

rijkvanzanten avatar Jul 26 '22 14:07 rijkvanzanten

@rijkvanzanten I agree for most users the default should be to readonly. However for me as a database admin and developer if there is an option to toggle readonly on and off then I expect it to work as such.

We do have cases were either old records from other systems need to be added with a specific Key and we want that new any item just continue where the last/highest ID was created last.

So either the option for readonly on the field needs to be removed or it needs to be respected :)

u12206050 avatar Jul 26 '22 15:07 u12206050

I'm fine with this as long as we have appropriate confirmations and warnings.

benhaynes avatar Jul 27 '22 03:07 benhaynes

@rijkvanzanten What about we set the readonly value to true by default for auto-complete fields, then it will have the same behaviour as per today, with the ability to unselect it for those that need to. Cause currently even as an admin I am unable to add an item with lets say an old id since we have imported only orders starting from 20000, but now I want to add an order with id 18000, and since I can't do that via UI I am forced to do it either via DB or API.

Btw, do I have to create a new PR once it is closed, or are you able to open it again?

u12206050 avatar Jul 28 '22 08:07 u12206050

Cause currently even as an admin I am unable to add an item with lets say an old id since we have imported only orders starting from 20000, but now I want to add an order with id 18000, and since I can't do that via UI I am forced to do it either via DB or API.

I get the use case, but I'm still a little worried about people not realizing the auto increment restrictions (waiting for the "I'm getting a duplicate ID error" issue being opened), but you're correct in that it's maybe a tad too restrictive if it's an explicit opt-in.

What about we set the readonly value to true by default for auto-complete fields, then it will have the same behaviour as per today, with the ability to unselect it for those that need to.

OK Deal 🀝

Btw, do I have to create a new PR once it is closed, or are you able to open it again?

You can keep pushing to the same branch (this PR), and I can reopen it πŸ™‚

rijkvanzanten avatar Jul 28 '22 13:07 rijkvanzanten

MSSQL might be an issue as it requires IDENTITY_INSERT to be turned off for manual insertion into identity column. πŸ€” Ref: https://docs.microsoft.com/en-us/sql/t-sql/statements/set-identity-insert-transact-sql?view=sql-server-ver15

licitdev avatar Jul 28 '22 18:07 licitdev

MSSQL might be an issue as it requires IDENTITY_INSERT to be turned off for manual insertion into identity column. πŸ€” Ref: https://docs.microsoft.com/en-us/sql/t-sql/statements/set-identity-insert-transact-sql?view=sql-server-ver15

Would think the developers setting this up would also know or be able to figure that out though?

u12206050 avatar Jul 28 '22 18:07 u12206050

Would think the developers setting this up would also know or be able to figure that out though?

You'd be surprised at the range of skill levels that you'd find in a widely used project like this.. 😬 Preventing footguns in the app is often a good idea. In this particular case, I'm hopeful that the error MSSQL throws is explicit enough for the user to be able to stack overflow it from there, but it isn't a great experience. It's always balancing act between flexibility and handholding though.

rijkvanzanten avatar Jul 28 '22 19:07 rijkvanzanten

@rijkvanzanten Have added a warning now that shows up when it is an auto_incremented field. Still have it though that if readonly isn't set, that the field can only be set during create, and not when being updated.

u12206050 avatar Jul 29 '22 08:07 u12206050

Heya, thanks for your PR! Our small team has been attempting to work through a backlog of external contributions for the better part of a year, but in that time many have become too stale to merge or the change is no longer desired/relevant.. We've updated the contributing guidelines and are working on making more resources available to make sure that doesn’t happen again 😬 In the meantime, we'll have to close this PR for now..

rijkvanzanten avatar Apr 10 '23 20:04 rijkvanzanten