feat: add option to enforce unique attribute #49

hbagdi · 2018-10-28T22:08:30Z

Enforcing Uniqueness on an Index can go a long way since higher-level
application code doesn't need to add explicit checks for it.

To keep the change backwards compatible, changes have been made to
Insert() method to respect Unique constraints.

Insert will not allow update if EnforceUnique is set to true on the id
schema.
If any other schema of the table has EnforceUnique, updates using
Insert() will error out if the same value exists in the schema.

jefferai · 2018-10-29T00:05:21Z

@vishalnayak almost certainly of interest to you

vishalnayak · 2018-10-29T15:39:02Z

This is eliminating the need of higher level application code to check that the field value being inserted is not already being used in another object that is already inserted, which seems to be nice. However, this PR is also overloading another meaning to EnforceUnique, which is that a value can only be inserted once and that an update to the same field errors out even if the value being inserted is "unique". I am not sure if that is the right way to go.

vishalnayak · 2018-10-29T15:43:41Z

I think that updates should be allowed as long as the value is not used elsewhere. Currently, "other" fields in the object cannot be updated because the uniqueness constraint on one field disallows updating the object altogether.

hbagdi · 2018-10-29T16:00:16Z

I think that updates should be allowed as long as the value is not used elsewhere. Currently, "other" fields in the object cannot be updated because the uniqueness constraint on one field disallows updating the object altogether.

@vishalnayak I expected this to come up and when I initially took a stab, updates seemed complicated to resolve.

I had introduced an Update() method in a previous incarnation of this commit, which took care of this case.

Giving a second look at the code, it seems that updates could be allowed and be a little more smarter, but might be a little expensive as well.

An unrelated question, not sure who can answer @jefferai or @vishalnayak,

Can I remove EnforceUnique, and overload the meaning of Unique? This could be a breaking change, hence the concern.

vishalnayak · 2018-10-29T20:38:52Z

@hbagdi I do strongly feel that update is a very common use case for an object (at least Vault uses memdb in such a manner). I also wonder about the double iteration of tableSchema.Indexes and if it can be avoided. Definitely curious to know what would be the overhead.

Regarding merging the functionality of EnforceUnique within Unique, you are right in thinking that it would be a backwards incompatible change. It would only complain those users who have used the Unique incorrectly by inserting multiple objects without honoring the Unique setting. But, it can't be known how many projects are using this and in which ways. Being a person who has not maintained this repository before, I can't weigh in strongly. But I am against breaking the existing behavior.

vishalnayak · 2018-11-08T19:28:05Z

txn.go

-	// On an update, there is an existing object with the given
-	// primary ID. We do the update by deleting the current object
-	// and inserting the new object.
+	if update && idSchema.Unique && idSchema.EnforceUnique {


It is not certain at this point if this PR will get merged or not. In any case, this check should certainly go away.

hbagdi · 2018-11-17T04:53:52Z

The problem here is if you allow updates using Insert() method, then it leads to a confusing API.

Currently, Insert() can insert or update an entity.

Also, EnforceUnique though not present in the package's master branch, is actually enforced on the idSchema, because duplicates in idSchema are updated/overwritten.

Now, if a user of this application would like to get an error if there is duplicate ID during an insert, then update operations cannot be supported using the same Insert().

I'd happy to add an Update() if that is an acceptable solution.

Let me know your thoughts!

hbagdi · 2018-12-03T23:35:02Z

Friendly ping on this to keep moving forward. Thanks!

hbagdi · 2019-04-18T03:20:27Z

@jefferai @vishalnayak Would you guys be interesting in moving this forward?

jefferai · 2019-05-08T22:28:14Z

@hbagdi We are, but please address the issue Vishal left -- we should explicitly not allow EnforceUnique on the ID. That way we can know if it's an insert or an update via the current API.

This check can be done as part of the schema validation.

Also probably the error should be a defined value for an enforce uniqueness violation to help the application understand what the issue is.

hbagdi · 2019-05-13T22:57:20Z

@hbagdi We are, but please address the issue Vishal left -- we should explicitly not allow EnforceUnique on the ID. That way we can know if it's an insert or an update via the current API.

For my use-case, I specifically want to throw an error if when an Insert() is actually performing an Update and not an Insert. If we don't actually EnforceUnique on ID, then it is not possible to support this specific use-case. 😕
Anything we can do to solve both of our use-cases?

jefferai · 2019-05-14T13:31:11Z

Can you describe your actual use-case then? Is it purely to have a knob to never allow updates at all? Because that probably makes more sense at a table level.

hbagdi · 2019-05-14T14:32:08Z

Can you describe your actual use-case then? Is it purely to have a knob to never allow updates at all? Because that probably makes more sense at a table level.

I'm using memdb to store a cache of SQL table and then performing CRUD operations on it.
The use case does need to update the table but update and insert have different semantics.
It is needed that memdb throws an error if we try to insert a duplicate entry into the table, which is currently treated as an update or rather upsert.

Is it possible to release a v1 of this package and then add these APIs in the v2 version of the library, such that we keep the backwards compat by default (and give users to change the behavior as an opt-in, with EnforceUnique on ID)?

jefferai · 2019-05-14T14:44:08Z

The use case does need to update the table but update
It is needed that memdb throws an error if we try to insert a duplicate entry into the table, which is currently treated as an update or rather upsert.

But how do you propose to actually do this? Those seem fundamentally at odds with the current proposal. If you want to update you need to call Insert with an ID so that we know what to update. If EnforceUnique is true then this will error, meaning you can't ever actually update.

If there was an Update call coupled with a table option to disallow updates via insert, then that would probably do what you want. But it doesn't really make sense to disallow updates per index -- objects are put into tables, indices are just built on top of the objects.

hbagdi · 2019-05-14T15:43:00Z

Indeed. In a very early incarnation of this PR, I had introduced an Update() function which was the path to be taken for updates and the Insert() would fail if it was being used for Update().

But it doesn't really make sense to disallow updates per index -- objects are put into tables, indices are just built on top of the objects.

Correct. We will need to use the index to check for duplicates though.

hbagdi · 2019-07-10T18:25:20Z

Friendly ping.

jefferai · 2019-07-10T18:41:45Z

Hi there! What do you need on our end? It sounded like we were in agreement on the way forward but there have been no updates to the code.

hbagdi · 2019-09-21T18:08:47Z

Apologies for dropping the balls on this one, it has been a busy year.

If there was an Update call coupled with a table option to disallow updates via insert, then that would probably do what you want. But it doesn't really make sense to disallow updates per index -- objects are put into tables, indices are just built on top of the objects.

Is this an acceptable solution to this library?

I think to solve this problem, it is necessary to distinguish between an Insert and Update, otherwise, it is not possible to detect if the caller intended an insert or an update using just a single Insert().

hbagdi · 2019-11-04T02:31:36Z

This has stalled and is out of date.
If needed, I'll open up again in future with a better implementation and reasoning for design.

jefferai · 2019-11-08T16:53:35Z

Hi @hbagdi ,

Just to be clear, is it that you don't have time to get this across the finish line? I think we were just waiting on updates from you. It seems like we'd all agreed on the need for Update instead of changing Insert's behavior.

jefferai · 2019-11-08T16:53:49Z

Reopening for tracking.

hbagdi · 2019-11-08T20:31:01Z

Okay, backwards compatibility remains a challenge but I'll work on it.
I worked on the original Update method more than a year ago but I'm much more familiar with this code-base now. I'll push it during the holidays.

jefferai · 2019-11-08T20:36:55Z

Is backwards compat a challenge with a separate Update method and non-default option to disallow updates via insert?

hbagdi · 2019-11-08T20:42:05Z

Is backwards compat a challenge with a separate Update method and non-default option to disallow updates via insert?

No, then that shouldn't be a problem. I now know what to do. I'll ping back if I run into another blocker.

Enforcing Uniqueness on an Index can go a long way since higher-level application code doesn't need to add explicit checks for it. To keep the change backwards compatible, changes have been made to `Insert()` method to respect Unique constraints. Insert will not allow update if `EnforceUniqueness` is set to true on the id schema. If any other schema of the table has EnforceUnique, updates using `Insert()` will error out if the same value exists in the schema. A new `Update()` has been added to allow for updates when `EnforceUniqueness` is set.

hbagdi · 2019-11-23T04:01:50Z

@jefferai I updated the PR with what we are talking above.
It is not merge-ready (needs more tests) but I can polish it up if this aligns with what you've in mind.

jefferai · 2019-11-23T20:26:32Z

txn.go

@@ -213,6 +216,15 @@ func (txn *Txn) Insert(table string, obj interface{}) error {
 			}
 		}

+		if indexSchema.EnforceUniqueness && indexSchema.Unique {


Isn't this redundant with the check above?

jefferai · 2019-11-23T20:27:30Z

schema.go

@@ -93,7 +93,9 @@ type IndexSchema struct {
 	// exist from a structure.
 	AllowMissing bool

-	Unique  bool
+	Unique            bool
+	EnforceUniqueness bool


I think the naming of this is a bit confusing, like, why wouldn't the Unique val already require this? Maybe something like DisallowUpdatesViaInsert?

hashicorp-cla · 2022-03-12T16:50:57Z

All committers have signed the CLA.

hbagdi force-pushed the feat/enforce-unique branch from 2492bb4 to 45e8b03 Compare October 28, 2018 22:11

hbagdi mentioned this pull request Oct 28, 2018

Unique constraint on an index should throw an error if an item already exists #7

Open

vishalnayak reviewed Nov 8, 2018

View reviewed changes

hbagdi closed this Nov 4, 2019

jefferai reopened this Nov 8, 2019

hbagdi force-pushed the feat/enforce-unique branch from 45e8b03 to 44c2c01 Compare November 23, 2019 04:00

jefferai reviewed Nov 23, 2019

View reviewed changes

feat: add option to enforce unique attribute #49

Are you sure you want to change the base?

feat: add option to enforce unique attribute #49

Uh oh!

Conversation

hbagdi commented Oct 28, 2018

Uh oh!

jefferai commented Oct 29, 2018

Uh oh!

vishalnayak commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vishalnayak commented Oct 29, 2018

Uh oh!

hbagdi commented Oct 29, 2018

Uh oh!

vishalnayak commented Oct 29, 2018

Uh oh!

vishalnayak Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

hbagdi commented Nov 17, 2018

Uh oh!

hbagdi commented Dec 3, 2018

Uh oh!

hbagdi commented Apr 18, 2019

Uh oh!

jefferai commented May 8, 2019

Uh oh!

hbagdi commented May 13, 2019

Uh oh!

jefferai commented May 14, 2019

Uh oh!

hbagdi commented May 14, 2019

Uh oh!

jefferai commented May 14, 2019

Uh oh!

hbagdi commented May 14, 2019

Uh oh!

hbagdi commented Jul 10, 2019

Uh oh!

jefferai commented Jul 10, 2019

Uh oh!

hbagdi commented Sep 21, 2019

Uh oh!

hbagdi commented Nov 4, 2019

Uh oh!

jefferai commented Nov 8, 2019

Uh oh!

jefferai commented Nov 8, 2019

Uh oh!

hbagdi commented Nov 8, 2019

Uh oh!

jefferai commented Nov 8, 2019

Uh oh!

hbagdi commented Nov 8, 2019

Uh oh!

hbagdi commented Nov 23, 2019

Uh oh!

jefferai Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

jefferai Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

hashicorp-cla commented Mar 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vishalnayak commented Oct 29, 2018 •

edited

Loading

hashicorp-cla commented Mar 12, 2022 •

edited

Loading