Add initial support for diffing conflicts

[trodge]

Helps with https://github.com/hashicorp/terraform-provider-google/issues/18902

Release Note Template for Downstream PRs (will be copied)

Additional data for comparing field conflicts in the diff processor

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

[github-actions[bot]]

@trodge, this PR is waiting for action from you. Please address any comments or change requests, or re-request review from a core reviewer if no action is required.

If no action is taken, this PR will be closed in 28 days.

This notification can be disabled with the disable-automatic-closure label.

[melinath]

I think the automation detected a comment from a core team member even though it's from the PR author

[trodge]

1. Agree that "conflicts" is a slightly confusing name, but it can be argued that all four of AtLeastOneOf RequiredWith ExactlyOneOf and Conflicts are types of conflicts in that they all restrict how different fields can be set relative to each other.

2. I think only having Old and New slices will not allow us to do meaningful analysis, because each set will need to be compared with every other set. It's possible there could be some readability benefit to doing this separately from diffFields, but the benefit of doing all of it here is that we don't need to do additional work to detect if any of the conflict sets of a field have changed.

3. The main benefit to using sets all the way down is being 100% sure that we don't have duplicate fields within a set of fields, or duplicate sets of fields within a resource without needing to iterate all the sets at every step.

4. Checking for breaking changes will require analyzing the sets themselves, but the keys will still be useful in identifying which sets to compare. Once we have Added Removed Expanded and Shrank, we will be able to write rules looking at, for example, Added or Expanded ExactlyOneOf sets.

5. I'm not sure how we would detect breaking changes without a detailed view of how the conflict sets are changing. That being said, it might be better to put this in a different subpackage of the diff processor so that the interface is more separate from the implementation.

[melinath]

1. Agree that "conflicts" is a slightly confusing name, but it can be argued that all four of AtLeastOneOf RequiredWith ExactlyOneOf and Conflicts are types of conflicts in that they all restrict how different fields can be set relative to each other.

I think that's a bit of a stretch - "conflicting" and "restricting" are not really the same.

2. I think only having Old and New slices will not allow us to do meaningful analysis, because each set will need to be compared with every other set. It's possible there could be some readability benefit to doing this separately from diffFields, but the benefit of doing all of it here is that we don't need to do additional work to detect if any of the conflict sets of a field have changed.

I believe that constructing an accurate list of sets that were added, removed, expanded, or shrunk requires comparing every set with every other set regardless - whether we do it here or in the breaking change detector. The benefit of doing it here would be that we could reuse the computation (but that's only relevant if we have multiple things that need the computation.)

3. The main benefit to using sets all the way down is being 100% sure that we don't have duplicate fields within a set of fields, or duplicate sets of fields within a resource without needing to iterate all the sets at every step.

I don't think I understand this argument. If we construct the top-level sets as we iterate over fields, how could we end up missing fields? We do need to deduplicate fieldsets, but we don't need to store the keys past the deduplication (because they aren't useful for comparison.)

4. Checking for breaking changes will require analyzing the sets themselves, but the keys will still be useful in identifying which sets to compare. Once we have Added Removed Expanded and Shrank, we will be able to write rules looking at, for example, Added or Expanded ExactlyOneOf sets.

I'm not convinced that the keys are useful for comparison. For example:

• Initial set is field1, field3. field2 is added. The key can't be used to determine that a field was added.
• Initial set is field1, field2, field3. field2 is removed. The key can't be used to determine that field2 was removed.

Hypothetically, we could try to use information about what fields in the set were added / removed (as part of the field diffing process) to build keys for comparison, but that has limitations. For example,

• Initial set is field1, field2. field3 and field4 are added. Neither new field can find the "old" version of the set because it changed by two keys.
• Same thing happens if both are removed, or if one is added and one removed.

I believe that trying to fix those limitations will end up adding additional complexity and eventually just end up comparing all the sets with each other (but in a less clear way). And I don't think that we have evidence that the set comparison is prohibitively slow at this time. (And if it were, I think it would be easier to approach the optimization as a problem of efficiently comparing sets rather than trying to avoid comparing sets.)

5. I'm not sure how we would detect breaking changes without a detailed view of how the conflict sets are changing. That being said, it might be better to put this in a different subpackage of the diff processor so that the interface is more separate from the implementation.

My proposal is that the breaking change detection would use the old / new fieldsets to build a detailed view of how the conflict sets are changing. We could also do it here. I would have a slight preference to put it in the breaking change detection code first, since we only have that one use case so far, but I could be convinced to put it in the differ instead.

---

Let me know what you think!

[trodge]

I think you're right that this PR is potentially a premature optimization. I'll work on a simpler, more brute force solution when I have time.

[github-actions[bot]]

@trodge, this PR is waiting for action from you. Please address any comments or change requests, or re-request review from a core reviewer if no action is required.

If no action is taken, this PR will be closed in 28 days.

This notification can be disabled with the disable-automatic-closure label.

[github-actions[bot]]

@trodge, this PR is waiting for action from you. If no action is taken, this PR will be closed in 14 days.

Please address any comments or change requests, or re-request review from a core reviewer if no action is required.

This notification can be disabled with the disable-automatic-closure label.

[github-actions[bot]]

@trodge, this PR is waiting for action from you. If no action is taken, this PR will be closed in 2 weekdays.

Please address any comments or change requests, or re-request review from a core reviewer if no action is required.

This notification can be disabled with the disable-automatic-closure label.

[github-actions[bot]]

@trodge, this PR is being closed due to inactivity.