FieldMask JSON read/write (snake_case <-> camelCase) is non-conforming

[jcready]

From field_mask_util_test.cc:

TEST_F(SnakeCaseCamelCaseTest, SnakeToCamel) {
  EXPECT_EQ("fooBar", SnakeCaseToCamelCase("foo_bar"));
  EXPECT_EQ("FooBar", SnakeCaseToCamelCase("_foo_bar"));
  EXPECT_EQ("foo3Bar", SnakeCaseToCamelCase("foo3_bar"));
  // No uppercase letter is allowed.
  EXPECT_EQ("#FAIL#", SnakeCaseToCamelCase("Foo"));
  // Any character after a "_" must be a lowercase letter.
  //   1. "_" cannot be followed by another "_".
  //   2. "_" cannot be followed by a digit.
  //   3. "_" cannot appear as the last character.
  EXPECT_EQ("#FAIL#", SnakeCaseToCamelCase("foo__bar"));
  EXPECT_EQ("#FAIL#", SnakeCaseToCamelCase("foo_3bar"));
  EXPECT_EQ("#FAIL#", SnakeCaseToCamelCase("foo_bar_"));
}


TEST_F(SnakeCaseCamelCaseTest, CamelToSnake) {
  EXPECT_EQ("foo_bar", CamelCaseToSnakeCase("fooBar"));
  EXPECT_EQ("_foo_bar", CamelCaseToSnakeCase("FooBar"));
  EXPECT_EQ("foo3_bar", CamelCaseToSnakeCase("foo3Bar"));
  // "_"s are not allowed.
  EXPECT_EQ("#FAIL#", CamelCaseToSnakeCase("foo_bar"));
}

FieldMask.toJson() (snake_case -> camelCase)

The existing implementation fails tests 2 and 3 for Any character after a "_" must be a lowercase letter. as these do not throw errors:

// 2. "_" cannot be followed by a digit.
FieldMask.toJson({ paths: ['foo_3bar'] });

// 3. "_" cannot appear as the last character.
FieldMask.toJson({ paths: ['foo_bar_'] });

I believe this could trivially be fixed with the following change to packages/plugin/src/message-type-extensions/well-known-types.ts

+                 const invalidFieldMaskJsonRegex = /[A-Z]|(_([.0-9_]|$))/g;
                  return message.paths.map(p => {
-                     if (p.match(/_[0-9]?_/g) || p.match(/[A-Z]/g))
+                     if (invalidFieldMaskJsonRegex.test(p))

FieldMask.fromJson() (camelCase -> snake_case)

The existing implementation fails the second expectation:

expect(FieldMask.fromJson('FooBar')).toEqual({ paths: ['_foo_bar'] });
// Expected $.paths[0] = 'foo_bar' to equal '_foo_bar'.

This failure is purely due to this one line but the git blame is so old and part of a commit simply titled "transferring to github.com" that it has become a classic Chesterton's Fence situation.

protobuf-ts/packages/plugin/src/message-type-extensions/well-known-types.ts

Line 382 in 3eea598

return (sc[0] === "_") ? sc.substring(1) : sc;

Again, I believe this could be trivially fixed.

-                      return (sc[0] === "_") ? sc.substring(1) : sc;
+                      return sc; 

[hugebdu]

@timostamm any plans/thoughts to align it?
10x

[timostamm]

@hugebdu, we should match the C++ implementation. It's very likely that protobuf-es has the same issue.
I'm strapped for time at the moment - contributions are most welcome. It should be fairly straight-forward to replicate the test cases and go from there.

[hugebdu]

@timostamm
I can try submitting a PR, however that's a breaking change.
Do you want to add an option to control the field mask casing or this will be a major release?

[hugebdu]

@timostamm?