Add message branding with types

[odashevskii-plaid]

I've originally commented here on why this is important; in a nutshell, the existing layout when messages are fully decoupled from their types doesn't work in situations when you need to reflect on a message which came from elsewhere and its exact type is unknown.

Here's a stripped-down example:

// PROTO SOURCE

// Data classification levels
enum Classification {
  // Non-sensitive data concerning Big Corp systems or users
  CLASSIFICATION_NON_SENSITIVE = 0;
  // Sensitive data concerning either consumers or developers (users of Big Corp)
  CLASSIFICATION_USER_SENSITIVE = 1;
}

extend google.protobuf.FieldOptions {
  // specifies data classfication
  Classification classification = 50001;
}

message Client {
  string client_id = 1;
  string secret = 2 [(annotations.classification) = CLASSIFICATION_USER_SENSITIVE];
}

// TS CODE

const client1 = Client.create({ ... });
const client2 = Client.create({ ... });

enterpriseGradeLogger.info("got something", {
  foo: "bar",
  clients: [client1, client2],
});

The second argument of enterpriseGradeLogger is an arbitrary object; recursive redaction is applied, and for any protobuf messages, redaction is based on field annotations.

With this PR, redaction logic could check if reflectionIsProtoMessage returns true and proceed with extracting annotations if so.

[timostamm]

Thank you for this, @odashevskii-plaid. I've run into cases like your logger example several times already. I've added a comment about the type property and my thoughts on it.

As far as I see, this could be a first step into this direction. I'm okay with it not being part of the generated interfaces for a start. I'm also okay with not caring about IMessageType.is, isAssignable and equals for now. But some points should be addressed before we can sneak this in.

[odashevskii-plaid]

@timostamm thanks for the quick turnaround and detailed suggestions! I think I have addressed all your comments except adding [MESSAGE_TYPE]: ... to the generated code (but see my comment). Now I will try fixing create as well.

[timostamm]

Thanks so far, @odashevskii-plaid. create has been fixed in 0534891.

For context:

protobuf-ts/packages/plugin/src/message-type-extensions/create.ts

Lines 73 to 87 in 0534891

	makeMessageVariable(source: TypescriptFile,descriptor: DescriptorProto) {
	let messageType = this.interpreter.getMessageType(descriptor);
	let defaultMessage = messageType.create();
	return ts.createVariableStatement(
	undefined,
	ts.createVariableDeclarationList(
	[ts.createVariableDeclaration(
	ts.createIdentifier("message"),
	undefined,
	typescriptLiteralFromValue(defaultMessage)
	)],
	ts.NodeFlags.Const
	)
	);
	}

• during "compilation", we have a in-memory MessageType for the message we want to generate code for
• defaultMessage is created from this MessageType with create()
• typescriptLiteralFromValue(defaultMessage) turns the object into an AST

I don't think it's possible to simply add MESSAGE_TYPE to defaultMessage because typescriptLiteralFromValue only understands primitive values. You will probably have to manipulate the AST , or generate a new line of code that does defaultMessage.MESSAGE_TYPE = ....

[odashevskii-plaid]

@timostamm thanks for the suggestions, I was able to figure out the Compiler API, see 483882e. The final create body looks like this:

    // packages/test-generated/ts-out/google/type/color.ts
    create(value?: PartialMessage<Color>): Color {
        const message = { red: 0, green: 0, blue: 0 };
        (message as unknown as MessageTypeContainer<Color>)[MESSAGE_TYPE] = this;
        if (value !== undefined)
            reflectionMergePartial<Color>(this, message, value);
        return message;
    }

[timostamm]

Apologies for suggesting incomplete typing for containsMessageType earlier, @odashevskii-plaid. Have added a suggestion to address. If you could implement it, this is ready to merge.

[odashevskii-plaid]

@timostamm the new typing is cool; I've removed getMessageType...

[timostamm]

Thanks for putting up with my review 😅 @odashevskii-plaid

Released in v2.0.3!

[jcready]

@timostamm I understand the use-case here, but IMO this really clutters up the objects returned from .create() and .fromBinary(). Is it possible to make this some kind of flag/option during codegen to disable this for consumers that do not need to perform reflection on in-memory objects?

I realize this wasn't a breaking change, but trying to update our codebase from 2.0.2 to 2.0.3 has been made difficult because we have some unit tests that are doing things like expect(decodedMessage).toEqual(expectedOutput) which are no longer passing. We either need to loosen up our expectations (e.g. replace .toEqual() with something like .toMatchObject()) or we need to const expectedOutput = IMessageType.create({ ...previouslyValidPlainJSObject });. Either way it's difficult for me to explain to my colleagues why a patch version bump requires their tests to be updated.

[timostamm]

If tests break, this is a breaking change in my mind. But why do they break?

Symbol properties are unlike regular properties (not listed in Object.keys for example) and this test (just added) passes:

protobuf-ts/packages/runtime/spec/message-type.spec.ts

Lines 86 to 100 in 1f6771f

	it('create() creates expected object', () => {
	const msg = MyMessage.create({
	stringField: "hello world",
	});
	const exp: MyMessage = {
	boolField: false,
	messageMap: {},
	stringField: "hello world",
	repeatedInt32Field: [],
	result: {
	oneofKind: undefined
	},
	}
	expect(msg).toEqual(exp);
	})

It is using Jasmine 3.5. Could you share details about your setup?

[jcready]

We're using Jest v26. Docs for .toEqual() here: https://jestjs.io/docs/26.x/expect#toequalvalue

I tracked down Jest's implementation of .toEqual() to here: https://github.com/facebook/jest/blob/bc50e7f360ab1845abbaa0b3ad788caead0d3174/packages/expect/src/jasmineUtils.ts#L29-L38, but it does look like it's comparing enumerable symbols: https://github.com/facebook/jest/blob/bc50e7f360ab1845abbaa0b3ad788caead0d3174/packages/expect/src/jasmineUtils.ts#L208-L214

[jcready]

Does this symbol need to be enumerable? It seems like @odashevskii-plaid's use-case would still be met if it was non-enumerable, but perhaps his example was simplified and he would still need this to work after stuff like object spread.

[timostamm]

I disagree with Jest's choice and believe that symbols should only be compared with toStrictEqual. But Jest is very widely used. Release v2.0.4-alpha.0 in the next channel makes the property non-enumerable.

@odashevskii-plaid, you can still access the MESSAGE_TYPE like before. But if you copy a message with {...msg}, the property will be lost - please use MyMessage.clone() instead.

@jcready, it would be great if you could confirm the fix with Jest.

[jcready]

I can confirm that making the property non-enumerable fixes the issue. Thank you for the incredibly quick turnaround. I hope no one else is broken by making it non-enumerable.

[timostamm]

I can confirm that making the property non-enumerable fixes the issue.

Thanks! Released in v2.0.4.