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(sdk): context.OutputData assumed to be JSON #540

Closed
AlexCuse opened this issue Oct 14, 2020 · 7 comments · Fixed by #644
Closed

bug(sdk): context.OutputData assumed to be JSON #540

AlexCuse opened this issue Oct 14, 2020 · 7 comments · Fixed by #644
Assignees
Labels
2-medium priority denoting issues with cross-cutting project impact bug Something isn't working ireland

Comments

@AlexCuse
Copy link
Contributor

AlexCuse commented Oct 14, 2020

When a messagebus function pipeline completes, the marshaled byte array is assumed to be JSON. See: https://github.com/edgexfoundry/app-functions-sdk-go/blob/master/internal/trigger/messagebus/messaging.go#L111-L124

Not sure if this behavior is by design or not but looking at other messagebus code it appears to be a bug. Passing content type to the complete method looks simple enough, but I have some concern about changing signature of .Complete. Another way to do it might be to make the SDK aware of the content type in use (defaulting to JSON) and propagating that through trigger initialization as needed. I think I probably favor the latter approach, but I can't really think of a use case for varying the content type on a per message basis and may be missing something.

@AlexCuse
Copy link
Contributor Author

Looking at the MQTT trigger, could also check against the content itself to decide between JSON and CBOR like so:

https://github.com/edgexfoundry/app-functions-sdk-go/blob/master/internal/trigger/mqtt/mqtt.go#L141-L145

@AlexCuse
Copy link
Contributor Author

Configuring content type and defaulting to JSON would also work, either as its own field on MessageBusConfig or via Optional.

@AlexCuse
Copy link
Contributor Author

I've done this by configuring "OutputContentType" through MessageBus.Optional for now, but suspect it will be best to include more flexibility in the future.

Another option that I am coming to prefer might be to make content type available on the context and change OutputData to an interface{}. Then marshaling could be done by the SDK immediately prior to publication and spare the user from needing to match the configured content type in their application code.

@rsdmike rsdmike added 2-medium priority denoting issues with cross-cutting project impact bug Something isn't working labels Nov 6, 2020
@rsdmike
Copy link
Member

rsdmike commented Nov 6, 2020

We'll take a closer look at this to figure out the best solution. It won't be included for Hanoi, but should be in our next release.

@AlexCuse
Copy link
Contributor Author

This seems like a fine way to address the issue to me: d7502e4

Happy to submit a PR to add "context.ResponseContentType" awareness to the remaining triggers if there are no plans for something else in the works.

@rsdmike
Copy link
Member

rsdmike commented Jan 15, 2021

@AlexCuse That would be great!

@AlexCuse AlexCuse self-assigned this Jan 16, 2021
@AlexCuse AlexCuse linked a pull request Jan 16, 2021 that will close this issue
12 tasks
@AlexCuse
Copy link
Contributor Author

Looks like it was only needed for the messagebus trigger since MQTT leaves marshaling entirely to the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2-medium priority denoting issues with cross-cutting project impact bug Something isn't working ireland
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants