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

[Issue #12486][Python Client]JsonSchema encoding is not idempotent #12490

Merged
merged 2 commits into from
Dec 7, 2021

Conversation

tsturzl
Copy link
Contributor

@tsturzl tsturzl commented Oct 26, 2021

Fixes #12486

Motivation

JsonSchemas delete part of the Record which makes it impossible to reuse a Record. Resulting in a KeyError anytime you encode the message more than once.

Modifications

Copy on encoding the Record so modifications don't change the root object. Check keys before deleting so you don't get a KeyError.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

@eolivelli eolivelli added the doc-not-needed Your PR changes do not impact docs label Oct 26, 2021
@tsturzl tsturzl changed the title [Issue #12486][Python Client] [Issue #12486][Python Client]JsonSchema encoding is not idempotent Oct 26, 2021
@merlimat merlimat added this to the 2.10.0 milestone Oct 28, 2021
@merlimat merlimat added release/2.9.1 type/bug The PR fixed a bug or issue reported a bug labels Oct 28, 2021
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also a good idea to add a test to repro the issue, so that we're not going to break it in the future.

@tsturzl
Copy link
Contributor Author

tsturzl commented Oct 28, 2021

@merlimat That's a good point. I actually had completely overlooked the unit tests. If you'd like I can add that to this PR. I may not have time until next week however.

@tsturzl
Copy link
Contributor Author

tsturzl commented Dec 6, 2021

I cannot get the cpp client to build, and therefore I'm not able to run the test I wrote. I couldn't build via the readme, nor could I get the docker build to succeed. I don't really have the free time currently to delve into CMake to figure out what's going on. If I had to guess based on the error message, the docker build fails probably because the docker system dependencies hasn't been kept up with the CMake dependencies for Boost Python. As for why it's failing on my machine I'm running Ubuntu 20.04 and the guide states 16.04, so maybe there's a breaking change in the version of cmake or some other tool or dependency required?

@merlimat merlimat merged commit 2df53da into apache:master Dec 7, 2021
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
…ent (apache#12490)

* fix JsonSchema, copy data out to prevent modifying the reference object,
check keys before deleting them

* add unit test, but cannot test due to compilation failure for cpp lib
codelipenghui pushed a commit that referenced this pull request Dec 21, 2021
…12490)

* fix JsonSchema, copy data out to prevent modifying the reference object,
check keys before deleting them

* add unit test, but cannot test due to compilation failure for cpp lib

(cherry picked from commit 2df53da)
@codelipenghui codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.9.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python Client, JsonSchema encoding is not idempotent
5 participants