From fc1ec890e040ec6a74bc47f1365d0bd3c1994e4d Mon Sep 17 00:00:00 2001 From: Joshua Humphries <2035234+jhump@users.noreply.github.com> Date: Mon, 13 May 2024 17:01:00 -0400 Subject: [PATCH] Tweak configuration of JSON format rules for buf breaking (#2978) While writing docs up for these I realized a couple of things: 1. They really needed to be included in the `WIRE_JSON` category (for likely obvious reasons). 2. It is backwards-compatible to go from "best effort" to "allow", just not vice versa. --- private/buf/cmd/buf/buf_test.go | 12 ++++++------ .../bufpkg/bufcheck/bufbreaking/bufbreaking_test.go | 10 ---------- .../internal/bufbreakingcheck/bufbreakingcheck.go | 4 ++-- .../bufbreaking/internal/bufbreakingv1/vars.go | 2 ++ .../bufbreaking/internal/bufbreakingv1beta1/vars.go | 2 ++ .../bufbreaking/internal/bufbreakingv2/vars.go | 2 ++ 6 files changed, 14 insertions(+), 18 deletions(-) diff --git a/private/buf/cmd/buf/buf_test.go b/private/buf/cmd/buf/buf_test.go index f45b9c435a..0707b2ddd7 100644 --- a/private/buf/cmd/buf/buf_test.go +++ b/private/buf/cmd/buf/buf_test.go @@ -703,7 +703,6 @@ ENUM_NO_DELETE FILE FILE_NO_DELETE FILE Checks that files are not deleted. MESSAGE_NO_DELETE FILE Checks that messages are not deleted from a given file. SERVICE_NO_DELETE FILE Checks that services are not deleted from a given file. -ENUM_SAME_JSON_FORMAT FILE, PACKAGE Checks that enums have the same JSON format support. ENUM_SAME_TYPE FILE, PACKAGE Checks that enums have the same type (open vs closed). ENUM_VALUE_NO_DELETE FILE, PACKAGE Checks that enum values are not deleted from a given enum. EXTENSION_MESSAGE_NO_DELETE FILE, PACKAGE Checks that extension ranges are not deleted from a given message. @@ -732,12 +731,13 @@ FILE_SAME_RUBY_PACKAGE FILE, PACKAGE FILE_SAME_SWIFT_PREFIX FILE, PACKAGE Checks that files have the same value for the swift_prefix option. FILE_SAME_SYNTAX FILE, PACKAGE Checks that files have the same syntax. MESSAGE_NO_REMOVE_STANDARD_DESCRIPTOR_ACCESSOR FILE, PACKAGE Checks that messages do not change the no_standard_descriptor_accessor option from false or unset to true. -MESSAGE_SAME_JSON_FORMAT FILE, PACKAGE Checks that messages have the same JSON format support. ONEOF_NO_DELETE FILE, PACKAGE Checks that oneofs are not deleted from a given message. RPC_NO_DELETE FILE, PACKAGE Checks that rpcs are not deleted from a given service. +ENUM_SAME_JSON_FORMAT FILE, PACKAGE, WIRE_JSON Checks that enums have the same JSON format support. ENUM_VALUE_SAME_NAME FILE, PACKAGE, WIRE_JSON Checks that enum values have the same name. FIELD_SAME_JSON_NAME FILE, PACKAGE, WIRE_JSON Checks that fields have the same value for the json_name option. FIELD_SAME_NAME FILE, PACKAGE, WIRE_JSON Checks that fields have the same names in a given message. +MESSAGE_SAME_JSON_FORMAT FILE, PACKAGE, WIRE_JSON Checks that messages have the same JSON format support. FIELD_SAME_ONEOF FILE, PACKAGE, WIRE_JSON, WIRE Checks that fields have the same oneofs in a given message. FILE_SAME_PACKAGE FILE, PACKAGE, WIRE_JSON, WIRE Checks that files have the same package. MESSAGE_SAME_MESSAGE_SET_WIRE_FORMAT FILE, PACKAGE, WIRE_JSON, WIRE Checks that messages have the same value for the message_set_wire_format option. @@ -783,7 +783,6 @@ FILE_NO_DELETE FILE FILE_SAME_PACKAGE FILE Checks that files have the same package. MESSAGE_NO_DELETE FILE Checks that messages are not deleted from a given file. SERVICE_NO_DELETE FILE Checks that services are not deleted from a given file. -ENUM_SAME_JSON_FORMAT FILE, PACKAGE Checks that enums have the same JSON format support. ENUM_SAME_TYPE FILE, PACKAGE Checks that enums have the same type (open vs closed). ENUM_VALUE_NO_DELETE FILE, PACKAGE Checks that enum values are not deleted from a given enum. EXTENSION_MESSAGE_NO_DELETE FILE, PACKAGE Checks that extension ranges are not deleted from a given message. @@ -810,12 +809,13 @@ FILE_SAME_RUBY_PACKAGE FILE, PACKAGE FILE_SAME_SWIFT_PREFIX FILE, PACKAGE Checks that files have the same value for the swift_prefix option. FILE_SAME_SYNTAX FILE, PACKAGE Checks that files have the same syntax. MESSAGE_NO_REMOVE_STANDARD_DESCRIPTOR_ACCESSOR FILE, PACKAGE Checks that messages do not change the no_standard_descriptor_accessor option from false or unset to true. -MESSAGE_SAME_JSON_FORMAT FILE, PACKAGE Checks that messages have the same JSON format support. ONEOF_NO_DELETE FILE, PACKAGE Checks that oneofs are not deleted from a given message. RPC_NO_DELETE FILE, PACKAGE Checks that rpcs are not deleted from a given service. +ENUM_SAME_JSON_FORMAT FILE, PACKAGE, WIRE_JSON Checks that enums have the same JSON format support. ENUM_VALUE_SAME_NAME FILE, PACKAGE, WIRE_JSON Checks that enum values have the same name. FIELD_SAME_JSON_NAME FILE, PACKAGE, WIRE_JSON Checks that fields have the same value for the json_name option. FIELD_SAME_NAME FILE, PACKAGE, WIRE_JSON Checks that fields have the same names in a given message. +MESSAGE_SAME_JSON_FORMAT FILE, PACKAGE, WIRE_JSON Checks that messages have the same JSON format support. FIELD_SAME_CARDINALITY FILE, PACKAGE, WIRE_JSON, WIRE Checks that fields have the same cardinalities in a given message. FIELD_SAME_ONEOF FILE, PACKAGE, WIRE_JSON, WIRE Checks that fields have the same oneofs in a given message. FIELD_SAME_TYPE FILE, PACKAGE, WIRE_JSON, WIRE Checks that fields have the same types in a given message. @@ -858,7 +858,6 @@ EXTENSION_NO_DELETE FILE FILE_NO_DELETE FILE Checks that files are not deleted. MESSAGE_NO_DELETE FILE Checks that messages are not deleted from a given file. SERVICE_NO_DELETE FILE Checks that services are not deleted from a given file. -ENUM_SAME_JSON_FORMAT FILE, PACKAGE Checks that enums have the same JSON format support. ENUM_SAME_TYPE FILE, PACKAGE Checks that enums have the same type (open vs closed). ENUM_VALUE_NO_DELETE FILE, PACKAGE Checks that enum values are not deleted from a given enum. EXTENSION_MESSAGE_NO_DELETE FILE, PACKAGE Checks that extension ranges are not deleted from a given message. @@ -887,12 +886,13 @@ FILE_SAME_RUBY_PACKAGE FILE, PACKAGE FILE_SAME_SWIFT_PREFIX FILE, PACKAGE Checks that files have the same value for the swift_prefix option. FILE_SAME_SYNTAX FILE, PACKAGE Checks that files have the same syntax. MESSAGE_NO_REMOVE_STANDARD_DESCRIPTOR_ACCESSOR FILE, PACKAGE Checks that messages do not change the no_standard_descriptor_accessor option from false or unset to true. -MESSAGE_SAME_JSON_FORMAT FILE, PACKAGE Checks that messages have the same JSON format support. ONEOF_NO_DELETE FILE, PACKAGE Checks that oneofs are not deleted from a given message. RPC_NO_DELETE FILE, PACKAGE Checks that rpcs are not deleted from a given service. +ENUM_SAME_JSON_FORMAT FILE, PACKAGE, WIRE_JSON Checks that enums have the same JSON format support. ENUM_VALUE_SAME_NAME FILE, PACKAGE, WIRE_JSON Checks that enum values have the same name. FIELD_SAME_JSON_NAME FILE, PACKAGE, WIRE_JSON Checks that fields have the same value for the json_name option. FIELD_SAME_NAME FILE, PACKAGE, WIRE_JSON Checks that fields have the same names in a given message. +MESSAGE_SAME_JSON_FORMAT FILE, PACKAGE, WIRE_JSON Checks that messages have the same JSON format support. FIELD_SAME_DEFAULT FILE, PACKAGE, WIRE_JSON, WIRE Checks that fields have the same default value, if a default is specified. FIELD_SAME_ONEOF FILE, PACKAGE, WIRE_JSON, WIRE Checks that fields have the same oneofs in a given message. FILE_SAME_PACKAGE FILE, PACKAGE, WIRE_JSON, WIRE Checks that files have the same package. diff --git a/private/bufpkg/bufcheck/bufbreaking/bufbreaking_test.go b/private/bufpkg/bufcheck/bufbreaking/bufbreaking_test.go index c5bd64ff20..1c8ed4910b 100644 --- a/private/bufpkg/bufcheck/bufbreaking/bufbreaking_test.go +++ b/private/bufpkg/bufcheck/bufbreaking/bufbreaking_test.go @@ -53,13 +53,8 @@ func TestRunBreakingEnumSameJSONFormat(t *testing.T) { "breaking_enum_same_json_format", bufanalysistesting.NewFileAnnotation(t, "1.proto", 9, 1, 11, 2, "ENUM_SAME_JSON_FORMAT"), bufanalysistesting.NewFileAnnotation(t, "1.proto", 13, 1, 15, 2, "ENUM_SAME_JSON_FORMAT"), - bufanalysistesting.NewFileAnnotation(t, "2.proto", 5, 1, 7, 2, "ENUM_SAME_JSON_FORMAT"), - bufanalysistesting.NewFileAnnotation(t, "2.proto", 17, 1, 19, 2, "ENUM_SAME_JSON_FORMAT"), - bufanalysistesting.NewFileAnnotation(t, "3.proto", 5, 1, 7, 2, "ENUM_SAME_JSON_FORMAT"), - bufanalysistesting.NewFileAnnotation(t, "3.proto", 17, 1, 19, 2, "ENUM_SAME_JSON_FORMAT"), bufanalysistesting.NewFileAnnotation(t, "3.proto", 26, 3, 26, 52, "ENUM_SAME_JSON_FORMAT"), bufanalysistesting.NewFileAnnotation(t, "4.proto", 11, 1, 13, 2, "ENUM_SAME_JSON_FORMAT"), - bufanalysistesting.NewFileAnnotation(t, "4.proto", 29, 3, 29, 52, "ENUM_SAME_JSON_FORMAT"), ) } @@ -963,13 +958,8 @@ func TestRunBreakingMessageSameJSONFormat(t *testing.T) { "breaking_message_same_json_format", bufanalysistesting.NewFileAnnotation(t, "1.proto", 9, 1, 11, 2, "MESSAGE_SAME_JSON_FORMAT"), bufanalysistesting.NewFileAnnotation(t, "1.proto", 13, 1, 15, 2, "MESSAGE_SAME_JSON_FORMAT"), - bufanalysistesting.NewFileAnnotation(t, "2.proto", 5, 1, 7, 2, "MESSAGE_SAME_JSON_FORMAT"), - bufanalysistesting.NewFileAnnotation(t, "2.proto", 17, 1, 19, 2, "MESSAGE_SAME_JSON_FORMAT"), - bufanalysistesting.NewFileAnnotation(t, "3.proto", 5, 1, 7, 2, "MESSAGE_SAME_JSON_FORMAT"), - bufanalysistesting.NewFileAnnotation(t, "3.proto", 17, 1, 19, 2, "MESSAGE_SAME_JSON_FORMAT"), bufanalysistesting.NewFileAnnotation(t, "3.proto", 26, 3, 26, 52, "MESSAGE_SAME_JSON_FORMAT"), bufanalysistesting.NewFileAnnotation(t, "4.proto", 11, 1, 13, 2, "MESSAGE_SAME_JSON_FORMAT"), - bufanalysistesting.NewFileAnnotation(t, "4.proto", 29, 3, 29, 52, "MESSAGE_SAME_JSON_FORMAT"), ) } diff --git a/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingcheck/bufbreakingcheck.go b/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingcheck/bufbreakingcheck.go index ddfdf33a97..fd82bd1b9c 100644 --- a/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingcheck/bufbreakingcheck.go +++ b/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingcheck/bufbreakingcheck.go @@ -97,7 +97,7 @@ func checkEnumSameJSONFormat( return fmt.Errorf("unable to resolve value of %s feature: %w", featureField.Name(), err) } jsonFormat := descriptorpb.FeatureSet_JsonFormat(val.Enum()) - if previousJSONFormat != jsonFormat { + if previousJSONFormat == descriptorpb.FeatureSet_ALLOW && jsonFormat != descriptorpb.FeatureSet_ALLOW { add( enum, nil, @@ -1162,7 +1162,7 @@ func checkMessageSameJSONFormat( return fmt.Errorf("unable to resolve value of %s feature: %w", featureField.Name(), err) } jsonFormat := descriptorpb.FeatureSet_JsonFormat(val.Enum()) - if previousJSONFormat != jsonFormat { + if previousJSONFormat == descriptorpb.FeatureSet_ALLOW && jsonFormat != descriptorpb.FeatureSet_ALLOW { add( message, nil, diff --git a/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingv1/vars.go b/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingv1/vars.go index 58f8288e92..0a41612140 100644 --- a/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingv1/vars.go +++ b/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingv1/vars.go @@ -102,6 +102,7 @@ var ( "ENUM_SAME_JSON_FORMAT": { "FILE", "PACKAGE", + "WIRE_JSON", }, "ENUM_SAME_TYPE": { "FILE", @@ -281,6 +282,7 @@ var ( "MESSAGE_SAME_JSON_FORMAT": { "FILE", "PACKAGE", + "WIRE_JSON", }, "MESSAGE_SAME_MESSAGE_SET_WIRE_FORMAT": { "FILE", diff --git a/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingv1beta1/vars.go b/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingv1beta1/vars.go index 7565b1e7da..9cc3a6f1d1 100644 --- a/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingv1beta1/vars.go +++ b/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingv1beta1/vars.go @@ -98,6 +98,7 @@ var ( "ENUM_SAME_JSON_FORMAT": { "FILE", "PACKAGE", + "WIRE_JSON", }, "ENUM_SAME_TYPE": { "FILE", @@ -266,6 +267,7 @@ var ( "MESSAGE_SAME_JSON_FORMAT": { "FILE", "PACKAGE", + "WIRE_JSON", }, "MESSAGE_SAME_MESSAGE_SET_WIRE_FORMAT": { "FILE", diff --git a/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingv2/vars.go b/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingv2/vars.go index 7d2c8e9785..a93193a93f 100644 --- a/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingv2/vars.go +++ b/private/bufpkg/bufcheck/bufbreaking/internal/bufbreakingv2/vars.go @@ -101,6 +101,7 @@ var ( "ENUM_SAME_JSON_FORMAT": { "FILE", "PACKAGE", + "WIRE_JSON", }, "ENUM_SAME_TYPE": { "FILE", @@ -285,6 +286,7 @@ var ( "MESSAGE_SAME_JSON_FORMAT": { "FILE", "PACKAGE", + "WIRE_JSON", }, "MESSAGE_SAME_MESSAGE_SET_WIRE_FORMAT": { "FILE",