Skip to content

tofieldset: Add tests to show it already allow duplicates #249

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 6, 2023
Merged

tofieldset: Add tests to show it already allow duplicates #249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions typed/tofieldset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,31 @@ func (v *toFieldSetWalker) doScalar(t *schema.Scalar) ValidationErrors {
}

func (v *toFieldSetWalker) visitListItems(t *schema.List, list value.List) (errs ValidationErrors) {
// Keeps track of the PEs we've seen
seen := fieldpath.MakePathElementSet(list.Length())
// Keeps tracks of the PEs we've counted as duplicates
duplicates := fieldpath.MakePathElementSet(list.Length())
for i := 0; i < list.Length(); i++ {
child := list.At(i)
pe, _ := listItemToPathElement(v.allocator, v.schema, t, child)
if seen.Has(pe) {
if duplicates.Has(pe) {
// do nothing
} else {
v.set.Insert(append(v.path, pe))
duplicates.Insert(pe)
}
} else {
seen.Insert(pe)
}
}

for i := 0; i < list.Length(); i++ {
child := list.At(i)
pe, _ := listItemToPathElement(v.allocator, v.schema, t, child)
if duplicates.Has(pe) {
continue
}
v2 := v.prepareDescent(pe, t.ElementType)
v2.value = child
errs = append(errs, v2.toFieldSet()...)
Expand Down
21 changes: 18 additions & 3 deletions typed/toset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,12 @@ var fieldsetCases = []fieldsetTestCase{{
_P("setStr", _V("b")),
_P("setStr", _V("c")),
)},
{`{"setBool":[true,false]}`, _NS(
{`{"setStr":["a","b","c","a","b","c","c"]}`, _NS(
_P("setStr", _V("a")),
_P("setStr", _V("b")),
_P("setStr", _V("c")),
)},
{`{"setBool":[true,false,true]}`, _NS(
_P("setBool", _V(true)),
_P("setBool", _V(false)),
)},
Expand Down Expand Up @@ -244,6 +249,16 @@ var fieldsetCases = []fieldsetTestCase{{
_P("list", _KBF("key", "b", "id", 1), "key"),
_P("list", _KBF("key", "b", "id", 1), "id"),
)},
{`{"list":[{"key":"a","id":1,"nv":2},{"key":"a","id":2,"nv":3},{"key":"b","id":1},{"key":"a","id":2,"bv":true}]}`, _NS(
_P("list", _KBF("key", "a", "id", 1)),
_P("list", _KBF("key", "a", "id", 1), "key"),
_P("list", _KBF("key", "a", "id", 1), "id"),
_P("list", _KBF("key", "a", "id", 1), "nv"),
_P("list", _KBF("key", "a", "id", 2)),
Copy link
Contributor Author

@apelisse apelisse Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @jpbetz
This code is mostly irrelevant because we only use ToFieldSet on objects that are applied, and we should never allow apply to have duplicates (now, that being said, I'm curious if this whole change even makes sense?). I still wanted to illustrate that when someone will update with duplicates items, we will just give them ownership of the associative key and that's it, as a way to indicate that they own that key multiple times. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One drawback of this, but I don't know if we can solve it anyway, is that if someone applies one item, and then it gets updated into two items, that item won't be pruned when it's removed from the applied configuration, meaning it can no longer be removed with server-side apply. I think that's reasonable.

Copy link
Member

@alexzielenski alexzielenski Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is mostly irrelevant because we only use ToFieldSet on objects that are applied, and we should never allow apply to have duplicates (now, that being said, I'm curious if this whole change even makes sense?)

If apply can never have duplicates why are we making SMD ToFieldSet tolerate duplicates? How do Updates get the list of fields to own if not via ToFieldSet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If apply can never have duplicates why are we making SMD ToFieldSet tolerate duplicates?

Context, we're trying to solve #234

For built-ins, the listType=map is used a little loosely, and we don't have a great way to address that, except for accepting some duplicates. We still can't apply duplicates, but right now you can't apply an objects that has these duplicates.

How do Updates get the list of fields to own if not via ToFieldSet?

By using "Compare", they own the fields that they changed, since we don't know exactly what they intended to own.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still can't apply duplicates, but right now you can't apply an objects that has these duplicates.

Is ToFieldSet invoked on the final merged object, or the partial patch? If it is invoked on the partial patch aren't there no duplicates and this PR has no effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, that's more or less what I said in the PR body:

It's not super relevant because no one should ever be able to apply duplicates

But:

  1. This is for consistency, if we agree that this is how ownership of things that are duplicates, we should probably apply it everywhere, even if it's not relevant here.
  2. This is the first time we use that new pattern in the code so I want to make sure we're fine with it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if someone applies one item, and then it gets updated into two items, that item won't be pruned when it's removed from the applied configuration, meaning it can no longer be removed with server-side apply.

Why wouldn't it be pruned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because when you update to multiple items, it steals the ownership of the whole item and we don't prune things that you don't have ownership over.

Copy link
Member

@alexzielenski alexzielenski Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it steals the ownership of the whole item and we don't prune things that you don't have ownership over.

It sounds like you do own it if it steals ownership of the whole item? Do you only have ownership of one copy? Can you share example of the behavior for this. I would expect that you have ownerhsip of the whole duplicated field, and when you submit an UPDATE or PATCH that does not include it, then no one has ownership, and it is pruned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using sets, since it's a little simpler:
Apply, "applier": [1]
Update, "updater": [1, 1] # Updater steals ownership of 1
Apply, "applier": [] # 1 is not pruned because it's owned by updater.
Final object: [1, 1]

Copy link
Member

@alexzielenski alexzielenski Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like correct behavior to me. What if it was a map typed list with duplicate keys but unequal objects?

Apply, "applier": [ObjWithKeyA]
Update, "updater": [ObjWithKeyAButSlighlyDifferent, ObjWithKeyAButAlsoDifferent] # Should be three copies of 'A'?
Apply, "applier": [] # is A pruned?
Whats the final object

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point.

So first of all, Update replaces the whole list, so if you do

Update, "updater": [ObjWithKeyAButSlighlyDifferent, ObjWithKeyAButAlsoDifferent] # Should be three copies of 'A'?

you end-up with that exact object.

Second, the way this is meant to work, is that if you have duplicate, you own the key and the key only, so here's what happens in your scenario:

Apply, "applier": [ObjWithKeyA] -> applier owns ObjWithKeyA
Update, "updater": [ObjWithKeyAButSlighlyDifferent, ObjWithKeyAButAlsoDifferent] -> duplicates, updater owns just "KeyA" (none of the fields)
Apply, "applier": [] # is A pruned? no
Whats the final object
[ObjWithKeyAButSlighlyDifferent, ObjWithKeyAButAlsoDifferent] with KeyA owned by updater.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think thats reasonable

_P("list", _KBF("key", "b", "id", 1)),
_P("list", _KBF("key", "b", "id", 1), "key"),
_P("list", _KBF("key", "b", "id", 1), "id"),
)},
{`{"atomicList":["a","a","a"]}`, _NS(_P("atomicList"))},
},
}}
Expand All @@ -257,9 +272,9 @@ func (tt fieldsetTestCase) test(t *testing.T) {
v := v
t.Run(fmt.Sprintf("%v-%v", tt.name, i), func(t *testing.T) {
t.Parallel()
tv, err := parser.Type(tt.rootTypeName).FromYAML(v.object)
tv, err := parser.Type(tt.rootTypeName).FromYAML(v.object, typed.AllowDuplicates)
if err != nil {
t.Errorf("failed to parse object: %v", err)
t.Fatalf("failed to parse object: %v", err)
}
fs, err := tv.ToFieldSet()
if err != nil {
Expand Down