Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for Protocol Buffer references (imports) -- continued #202

Closed
wants to merge 54 commits into from
Closed

Implement support for Protocol Buffer references (imports) -- continued #202

wants to merge 54 commits into from

Conversation

erwinw
Copy link

@erwinw erwinw commented Apr 22, 2022

This PR is a continuation of #135 (which can now be closed) 1

What:

Add support for Protocol Buffer imports and schema references.

I believe this is a feature change without any breaking changes.

Why:

Imports are very useful when writing schema files using Protocol Buffers. And in order to register a schema that uses imports, we need to use Schema Register's references feature to let it know where to find those imported types. Otherwise, it fails to register the schema.

Previous work:

I believe this relates to #82 (and obviously builds on #135).

There are two other related PRs open in #92 and #121. As far as I can tell, the first extends getSchema to retrieve references from Schema Registry. The second extends register() by asking the user to provide the reference schema ids in userOpts. This PR attempts to extend register() to resolve references by looking them up (either directly by using the import name, or by mapping the imported name to a subject and optionally providing a specific version), and creating a Protocol Buffer schema that includes these.

How:

When registering a new schema, the register method now supports an optional user-provided importSubjects helper that maps import references to subjects. If not provided, the import reference is used directly as the subject (which is inline with the Java behaviour).

The new referencedSchemas SchemaHelper then fetches definitions of the imported schemas. This PR implements this helper for Protocol Buffers, but it could also be implemented for at least JSON schemas.

   referencedSchemas(schema: string): Promise<string[]>

The register method is then extended to use these helpers to get the schema references needed and verifies those are all registered recursively and then included as the references of the newly registered schema. Schema registry references are a list of [(Subject, Version)] .

docs.confluent.io/platform/current/schema-registry/serdes-develop/index.html#referenced-schemas

To get define and encode to work, I extended the ProtoSchema constructor so it can take references into account when building the schema instance. It fetches the Schema instances for the references and adds them to the Root context for the new Schema instance avoiding duplicates, so it has all the types it needs.

   constructor(schema: ConfluentSchema, opts?: ProtoOptions, references?: Schema[])

I then extended schemaFromConfluentSchema to pass through the referenced schema.

Testing

I have added tests to validate behaviour in a few cases:

  1. Register and encode protocol buffer v3 schema

  2. Register and encode protocol buffer v3 schema with import. The imported proto has an import itself, to check the recursive registration.

  3. Register and encode protocol buffer v3 schema with nested imports, to check for multiple recursive registrations.

TODO before merging

  1. Need to check whether we need to explicitly provide "common" imports and whether we can "cheat" and do so via the protobufjs library: https://github.com/protobufjs/protobuf.js/blob/master/tests/data/common.proto

Future work

In order to use this new functionality, we should implement support for Message Indexes. Since Protocol Buffers can contain multiple type definitions in a single schema file, each encoded Kafka message includes a Message Index that tells the consumer which Protocol Buffer Message type to use for parsing the message. This feature would change the binary wire format.

docs.confluent.io/platform/current/schema-registry/serdes-develop/index.html#wire-format

Footnotes

  1. The original PR, Implement support for Protocol Buffer references (imports)  #135 was written by @brinchj while working at @pleo-io. He has since left, and I'm resuming his work now, but for lack of write access to the original PR have recreated it in a @pleo-oss fork (to avoid the same problem from reoccurring while allowing the @pleo-io colleagues to also contribute to this PR).

Johan Brinch and others added 30 commits July 8, 2021 15:37
Confluent Schema Registry made a backwards incompatible API change without
changing the version of the API, so now we have to hack around this. Same
thing that was done for the Java client in confluentinc/schema-registry#1288

Fixes #155
Newer versions of the Ajv constructor aren't compatible
with the version we're using. Since we only care about the
compile method, we can require just that.
Newer versions of Ajv have made backwards incompatible changes
to the validation errors, so we have to have our own type that only
requires the things we need, and then handle both possible types.
Bumps [lodash](https://github.com/lodash/lodash) from 4.17.19 to 4.17.21.
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.17.19...4.17.21)

Signed-off-by: dependabot[bot] <[email protected]>
When mappersmith encounters a client-side error, the response body
can be just a string with the error message, rather than an object
with a message property. Currently the error message would be unavailable
to users of the schema registry.
dependabot bot and others added 24 commits April 19, 2022 11:15
Bumps [ajv](https://github.com/ajv-validator/ajv) from 6.10.2 to 6.12.6.
- [Release notes](https://github.com/ajv-validator/ajv/releases)
- [Commits](ajv-validator/ajv@v6.10.2...v6.12.6)

---
updated-dependencies:
- dependency-name: ajv
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [prismjs](https://github.com/PrismJS/prism) from 1.17.1 to 1.26.0.
- [Release notes](https://github.com/PrismJS/prism/releases)
- [Changelog](https://github.com/PrismJS/prism/blob/master/CHANGELOG.md)
- [Commits](PrismJS/prism@v1.17.1...v1.26.0)

---
updated-dependencies:
- dependency-name: prismjs
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [color-string](https://github.com/Qix-/color-string) from 1.5.3 to 1.9.0.
- [Release notes](https://github.com/Qix-/color-string/releases)
- [Changelog](https://github.com/Qix-/color-string/blob/master/CHANGELOG.md)
- [Commits](https://github.com/Qix-/color-string/commits/1.9.0)

---
updated-dependencies:
- dependency-name: color-string
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [url-parse](https://github.com/unshiftio/url-parse) from 1.4.7 to 1.5.4.
- [Release notes](https://github.com/unshiftio/url-parse/releases)
- [Commits](unshiftio/url-parse@1.4.7...1.5.4)

---
updated-dependencies:
- dependency-name: url-parse
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [shelljs](https://github.com/shelljs/shelljs) from 0.8.3 to 0.8.5.
- [Release notes](https://github.com/shelljs/shelljs/releases)
- [Changelog](https://github.com/shelljs/shelljs/blob/master/CHANGELOG.md)
- [Commits](shelljs/shelljs@v0.8.3...v0.8.5)

---
updated-dependencies:
- dependency-name: shelljs
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [ws](https://github.com/websockets/ws) from 7.2.3 to 7.5.7.
- [Release notes](https://github.com/websockets/ws/releases)
- [Commits](websockets/ws@7.2.3...7.5.7)

---
updated-dependencies:
- dependency-name: ws
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [postcss](https://github.com/postcss/postcss) from 7.0.17 to 7.0.39.
- [Release notes](https://github.com/postcss/postcss/releases)
- [Changelog](https://github.com/postcss/postcss/blob/7.0.39/CHANGELOG.md)
- [Commits](postcss/postcss@7.0.17...7.0.39)

---
updated-dependencies:
- dependency-name: postcss
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [tmpl](https://github.com/daaku/nodejs-tmpl) from 1.0.4 to 1.0.5.
- [Release notes](https://github.com/daaku/nodejs-tmpl/releases)
- [Commits](https://github.com/daaku/nodejs-tmpl/commits/v1.0.5)

---
updated-dependencies:
- dependency-name: tmpl
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [tar](https://github.com/npm/node-tar) from 4.4.10 to 4.4.19.
- [Release notes](https://github.com/npm/node-tar/releases)
- [Changelog](https://github.com/npm/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v4.4.10...v4.4.19)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [path-parse](https://github.com/jbgutierrez/path-parse) from 1.0.6 to 1.0.7.
- [Release notes](https://github.com/jbgutierrez/path-parse/releases)
- [Commits](https://github.com/jbgutierrez/path-parse/commits/v1.0.7)

---
updated-dependencies:
- dependency-name: path-parse
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [path-parse](https://github.com/jbgutierrez/path-parse) from 1.0.6 to 1.0.7.
- [Release notes](https://github.com/jbgutierrez/path-parse/releases)
- [Commits](https://github.com/jbgutierrez/path-parse/commits/v1.0.7)

---
updated-dependencies:
- dependency-name: path-parse
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [set-getter](https://github.com/doowb/set-getter) from 0.1.0 to 0.1.1.
- [Release notes](https://github.com/doowb/set-getter/releases)
- [Commits](https://github.com/doowb/set-getter/commits/0.1.1)

---
updated-dependencies:
- dependency-name: set-getter
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [glob-parent](https://github.com/gulpjs/glob-parent) from 5.1.1 to 5.1.2.
- [Release notes](https://github.com/gulpjs/glob-parent/releases)
- [Changelog](https://github.com/gulpjs/glob-parent/blob/main/CHANGELOG.md)
- [Commits](gulpjs/glob-parent@v5.1.1...v5.1.2)

---
updated-dependencies:
- dependency-name: glob-parent
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [url-parse](https://github.com/unshiftio/url-parse) from 1.5.4 to 1.5.7.
- [Release notes](https://github.com/unshiftio/url-parse/releases)
- [Commits](unshiftio/url-parse@1.5.4...1.5.7)

---
updated-dependencies:
- dependency-name: url-parse
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [url-parse](https://github.com/unshiftio/url-parse) from 1.5.7 to 1.5.10.
- [Release notes](https://github.com/unshiftio/url-parse/releases)
- [Commits](unshiftio/url-parse@1.5.7...1.5.10)

---
updated-dependencies:
- dependency-name: url-parse
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [prismjs](https://github.com/PrismJS/prism) from 1.26.0 to 1.27.0.
- [Release notes](https://github.com/PrismJS/prism/releases)
- [Changelog](https://github.com/PrismJS/prism/blob/master/CHANGELOG.md)
- [Commits](PrismJS/prism@v1.26.0...v1.27.0)

---
updated-dependencies:
- dependency-name: prismjs
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
@erwinw erwinw marked this pull request as ready for review April 22, 2022 11:49
@erwinw
Copy link
Author

erwinw commented Apr 22, 2022

Hi @maitsarv, regarding your comments on the original PR:

  1. It does not work with the { [SchemaType.PROTOBUF]: { messageName: 'CustomMessage' }} option. Tries to look for the messageName from every proto definition, should only be looking from the root.

I don't think this has anything to do with this PR, but is a pre-existing issue -- could that be correct?

  1. When defintion inherits some type through multiple different files, then it will get an error.

I've resolved this by deduplicating the definitions.

@erwinw
Copy link
Author

erwinw commented May 6, 2022

Hi @Nevon , sorry for the direct ping. Could I get you (to get someone) to have a look at this, please?

@ghost ghost closed this by deleting the head repository Feb 8, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants