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

[BUG] TableView with JSONSchema returns incorrect type in Get #1132

Closed
ojcm opened this issue Nov 16, 2023 · 2 comments · Fixed by #1133
Closed

[BUG] TableView with JSONSchema returns incorrect type in Get #1132

ojcm opened this issue Nov 16, 2023 · 2 comments · Fixed by #1133
Assignees
Labels
Milestone

Comments

@ojcm
Copy link
Contributor

ojcm commented Nov 16, 2023

Expected behavior

Using a TableView with a JSONSchema, the Get method should return a instance of the Go type provided via the TableViewOptions.SchemaValueType field.

Actual behavior

The type returned from the Get method is map[string]interface{} (and I presume it could also be []interface{}, string, float64, bool or nil depending on the raw JSON).

Steps to reproduce

The following test reproduces the issue, where exampleSchemaDef, testJSON, lookupURL and newTopicName are defined in the pulsar package test files.

func TestTableViewJSONSchema(t *testing.T) {
	var (
		schema = NewJSONSchema(exampleSchemaDef, nil)
		key    = "testKey"
		value  = testJSON{ID: 1, Name: "Pulsar"}
	)

	client, err := NewClient(ClientOptions{
		URL: lookupURL,
	})

	assert.NoError(t, err)
	defer client.Close()

	topic := newTopicName()

	producer, err := client.CreateProducer(ProducerOptions{
		Topic:  topic,
		Schema: schema,
	})
	assert.NoError(t, err)
	defer producer.Close()

	_, err = producer.Send(context.Background(), &ProducerMessage{
		Key:   key,
		Value: value,
	})
	assert.NoError(t, err)

	tv, err := client.CreateTableView(TableViewOptions{
		Topic:           topic,
		Schema:          schema,
		SchemaValueType: reflect.TypeOf(value),
	})
	assert.NoError(t, err)
	defer tv.Close()

	gotValue := tv.Get(key)
	// The following checks fail because `gotValue` has type map[string]interface{}
	assert.IsType(t, value, gotValue)
	assert.Equal(t, value, gotValue)
}

System configuration

Pulsar version: 3.1.0

@ojcm
Copy link
Contributor Author

ojcm commented Nov 16, 2023

I believe I have a fix for this. Once it has been reviewed by my colleagues I'll open a PR in this repo. I expect that will be later today or tomorrow.

@RobertIndie
Copy link
Member

@ojcm Thanks. I assign this issue to you.

@RobertIndie RobertIndie added this to the v0.12.0 milestone Nov 16, 2023
RobertIndie pushed a commit that referenced this issue Nov 20, 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`).
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 a pull request may close this issue.

2 participants