Schema compatibility rule for removing optional fields is inconsistent with other rules

[jpbetz]

In CompatibilityChecker the removing an optional field is not considered an error. As a result it is not considered an error to remove an optional field and then add a new optional field with the same field name as the field just removed, but with a different type. This is clearly inconsistent with the rule that it is a compatibility error to change the type of field.

Consider this case:

• An optional field, let's call it 'title' of type string exists in a record schema.
• Writers exist that produce data with "title", and readers exist that expect it to be of type string when present.
• The optional field is removed from the schema. This is not considered an error according to CompatibilityChecker.
• A new optional field is added to the schema, also called "title", but of type Title (a record type). This also is not considered an error according to CompatibilityChecker.

It is now possible to have writers producing either a string or a record for the 'title' field. It is also possible to have readers expecting either a string or a record for the field. They are clearly not compatible with one another.

I believe the correct fix it to make removing optional fields a compatibility error.

[sweeboonlim]

It should be an error.We should modify the error message to make it clear why it is considered incompatible in the included message, e.g. "because new field may be added with the same name but different type in the future"

[jpbetz]

Thanks Swee. I'll submit a pull request.

[kvidhani]

We have had discussions amongst ourselves internally at LinkedIn and we have always felt this needed to be fixed.

Let me circle back with others and get back to you.

Have you already submitted a pull request for this?

[jpbetz]

Nope, I have not submitted a pull request.

I'm glad you're already thinking about this. At Google, they deal with protobuf in a similar way. Once a tag field has been used in a .proto file, it is kept forever to prevent that tag from being accidentally misused for a new field. Granted, tags in protobuf are integers that are usually added in increasing order, so the odds that one will be reused if deleted is high, but still, in principal it is the same problem.

Pegasus currently has the notion of "deprecated". Maybe once this fix is in place, it would be worth having an even stronger form of obsoleteness? E.g. "obsolete": true ?

But here's what I would probably do:

+      appendMessage(CompatibilityMessage.Impact.BREAKS_NEW_AND_OLD_READERS,
+                    "new record removed optional fields %s, this allows a new field to be added " +
+                    "with the same name but different type in the future.",

The best existing "Impact" i found for this case is BREAKS_NEW_AND_OLD_READERS. This would make sense since it's the same "Impact" used for incompatible types, which is what we're really dealing with here.

It might be worth expanding on the message more.

[kvidhani]

Yes that message sounds like what Swee was suggesting too.

Do you want to submit a pull request for this eventually? Otherwise I will open a ticket which will be used for tracking this at work. I can't give an estimate on when we would fix it though.

[jpbetz]

I'll send a pull request in the next week for so.

On Monday, August 10, 2015, Karim Vidhani notifications@github.com wrote:

Yes that message sounds like what Swee was suggesting too.

Do you want to submit a pull request for this eventually? Otherwise I will
open a ticket which will be used for tracking this at work. I can't give an
estimate on when we would fix it though.

—
Reply to this email directly or view it on GitHub
#54 (comment).

[jpbetz]

Any update on this?

[eranl]

This issue represents a trade-off between safety (pull request) and flexibility (current code). The risk of an incompatible re-addition of a removed field is low, so arguably it’s worth taking in order to allow for cleaner schema evolution. For instance, the current approach encourages expressive schemas, even when a field is known upfront to have a short lifespan.

I propose adding an _allowFieldRemoval field to CompatibilityOptions, which would control whether removing an optional field is considered an error. That would allow users to choose the approach that works for them.

The current code is inconsistent in that it considers removing an optional field not an error, but considers removing a non-optional field with a default value an error. I propose fixing that by making the new option above also control whether removing a field with a default value is considered an error.

[jpbetz]

@eranl This sounds reasonable to me. We're basically saying that in addition to what we're proposing in this pull request, we will allow users to ignore a compatibility checker error they consider overly strict for their purposes, similar to how compilers and linters allow various warnings to be enabled/disabled.

[kvidhani]

How often, if ever, will people configure the compat checker before running it? Most people won't even know that this option to configure even exists. Therefore:

1. Does it even make sense to allow this sort of configuration.
2. If it does, what would the default be for the deletion of optional fields?

[jpbetz]

@eranl Do you have a existing use case?

@kvidhani The default would be to not allow the deletion of optional fields

[eranl]

Currently, the options are hard-coded, but I think they should be exposed through RestLiResourceModelCompatibilityChecker and PegasusPlugin, so that build scripts can control them.

To clarify, my expectation is that these options would configured at the rest.li installation level rather than by individual developers.

[jpbetz]

@kvidhani How should we proceed? Is this work that the rest.li team will take on?

[kvidhani]

I am comfortable with making this change (forbidding removing optional
fields), but I am not yet comfortable with introducing options because most
people generally won't want to deal with them.

Is this acceptable?

• Karim

On Thu, Jul 14, 2016 at 2:12 PM, Joe Betz notifications@github.com wrote:

@kvidhani https://github.com/kvidhani How should we proceed. Is this
work that the rest.li team will take on?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#54 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AIKnBYP3_H0ZI40tqwluJfyz2kwRziKgks5qVqY-gaJpZM4FnWp0
.

[jpbetz]

@kvidhani That sounds reasonable to me. If later there is a compelling use case for introducing options, they could always be added later.

[kvidhani]

I'm opening a jira to track this. How high of a priority do you think this
is?

• Karim

On Fri, Jul 15, 2016 at 12:19 PM, Joe Betz notifications@github.com wrote:

@kvidhani https://github.com/kvidhani That sounds reasonable to me. If
later there is a compelling use case for introducing options, they could
always be added later.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#54 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIKnBb96Ti-n0i0ev0lrg8_qCq1NALjiks5qV91egaJpZM4FnWp0
.

[jpbetz]

@kvidhani I'm not aware of anyone blocked by this. It would conclude some of the discussions around the consistency of the compatibility checker rules, but isn't really urgent. The good news is that the only work do be done review and then commit this PR.

[kvidhani]

@jpbetz We have a rest.li bug scheduling session coming up. I'll make sure this is scheduled and taken care of soon.

As part of this JIRA, we will review the pull request, commit it, update the documentation and close this discussion.

Will this suffice?

[jpbetz]

Sounds great to me. It's a tiny change (like 5 lines) so should literally take 5 minutes.

[jpbetz]

closing hopelessly stale issues I've opened