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 1132] Fix JSONSchema unmarshalling in TableView #1133

Merged

Conversation

ojcm
Copy link
Contributor

@ojcm ojcm commented Nov 16, 2023

Fixes #1132

Motivation

Fix issue #1132 - using JSONSchema with TableView

Modifications

  • Set a concrete type in the payload variable before JSON-unmarshalling into that variable. This allows the JSON package to identify and use the type rather than seeing it as interface{}.
  • Use reflect.Indirect(payload).Interface() when storing the payload and passing it to listeners to remove the pointer from reflect.New.
  • Add test coverage for TableView.Get covering all supported schema types.
  • Add test coverage for TableView.ForEachAndListen for JSONSchema.

Additional minor changes. They didn't seem worth their own MRs but I'm happy to split them out if that's better.

  • Correct typo in comments on TableView.ForEach and TableView.ForEachAndListen interface methods.
  • Correct TableView.ForEachAndListen comment to clarify that it continues to call the given action on future messages.
  • Correct formatting directive (%w -> %v) in error log tv.logger.Errorf("msg.GetSchemaValue() failed with %v; msg is %v", err, msg). (This indirectly calls fmt.Sprintf in logrus which doesn't support %w).

Verifying this change

  • Make sure that the change passes the CI checks.

This change has some existing test coverage from TestTableView.

This change has extended test coverage, see TestTableViewSchemas and TestForEachAndListenJSONSchema.

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

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: yes? This fixes broken behaviour in the API. It's theoretically possible that users could be relying on broken behaviour and are expecting the incorrect type from the TableView methods when using JSONSchema. In my opinion, this is unlikely and so shouldn't be considered a breaking change.
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable
  • If a feature is not applicable for documentation, explain why? not applicable
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@RobertIndie RobertIndie self-requested a review November 17, 2023 06:37
@RobertIndie RobertIndie added this to the v0.12.0 milestone Nov 17, 2023
@RobertIndie
Copy link
Member

Additional minor changes. They didn't seem worth their own MRs but I'm happy to split them out if that's better.

They are minor changes. I'm OK with including them in one PR. Thanks for your fix.

@ojcm ojcm force-pushed the tableview-JSONSchema-unmarshal-type branch from 2356a00 to 75c1e99 Compare November 17, 2023 08:51
@ojcm
Copy link
Contributor Author

ojcm commented Nov 17, 2023

I've made a couple of changes to the tests that failed the workflow:

  • Replaced a helper function that used generics pointer[T] with a non-generic version strPointer as it was only helpful for strings anyway.
  • Removed type assertions that copied locks with variants that don't. The lock copying wasn't an issue in this context but the linter can't know that.

@ojcm ojcm force-pushed the tableview-JSONSchema-unmarshal-type branch from 75c1e99 to 338d038 Compare November 17, 2023 10:59
@ojcm
Copy link
Contributor Author

ojcm commented Nov 17, 2023

One more change to replace anys with interface{}s to work with older Go versions.

@RobertIndie RobertIndie merged commit d457442 into apache:master Nov 20, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] TableView with JSONSchema returns incorrect type in Get
2 participants