search

roadrunner-server / roadrunner-plugins

📦 Home for the roadrunner plugins
MIT License
25 stars 9 forks source link

bug?(amqp): make sure that rr_header is not rejected #59

Closed wodor closed 2 years ago

wodor commented 2 years ago

Fixes the problem of rr_header being ignored regardless of its content, as it never is a byte array.

Reason for This PR

When I publish a message via amqp it is correctly consumed by roadrunner, but the headers are always empty when I try to access them in PHP with HeadersTrait#getHeaders()

I was using an external message producer to test this behaviour. After applying the changes (and formatting the rr_header JSON properly) I was able to get the headers

Description of Changes

The execution never went past through the []byte type assertion. I have changes the type assertion to string and then converted the string to byte array required by json.Unmarshall

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

PR Checklist

[Author TODO: Meet these criteria.] [Reviewer TODO: Verify that these criteria are met. Request changes if not]

rustatian commented 2 years ago

Hey @wodor , thanks for the PR. Actually, this is not a fix, because if you change the payload type into string RR won't be able to unmarshal its own payload -> https://github.com/spiral/roadrunner-plugins/blob/master/amqp/amqpjobs/item.go#L183. This is much more related to your particular header. The header is required to be a byte (uint8) array.

rustatian commented 2 years ago

@wodor Did you use roadrunner-jobs library? Or you own?

rustatian commented 2 years ago

@wodor closed due to inactivity.

wodor commented 2 years ago

Hey @rustatian Yes, I cloned roadrunner-jobs project and started to play with it, to consume messages produced elsewhere (via RabbitMQ management). Figured out that I need to add rr_id etc. But I never received headers added in rr_headers.

Then, to understand what is going on, I cloned roadrunner-binary and copied the amqp plugin into that codebase and registered my version of amqp plugin. Compiled it and copied the binary into roadrunner-jobs project.

image

sorry for the delay, I missed GH notifications

rustatian commented 2 years ago

Hey @wodor . np 😃 . I guess your issue, that the rabbitmq management system sends the header payload as a string, not as a slice of bytes. We don't have a special case for that. Also, thanks for noting that we have to improve our documentation about fields used as keys in the rabbitmq headers.

wodor commented 2 years ago

Yeah, I realise that I tried to use the roadrunner jobs in a non standard way. If you point me to where this documentation should be written I’ll draft something, got the code open.

rustatian commented 2 years ago

@wodor You may open a PR to the https://github.com/spiral/roadrunner-docs/blob/master/beep-beep/jobs.md. Will be very helpful 👍🏻

wodor commented 2 years ago

Hmm, I'm not sure if I can find a relevant place in that doc. I would add a note like this, but I'm not sure if it is relevant to amqp or to jobs in general:

Producing jobs from outside

Have in mind that that roadrunner-jobs is created to consume messages created by itself (or other Spiral components). If you want to create a job in a queue from outside, you need to make sure that message has the required metadata, for amqp it would be expressed as message headers:

Other fields (normally set from PHP code with methods like withDelay()) are

You have to be careful about the format of the content of these headers. For example, amqp expects that rr_headers are []byte not a string - it is not possible to do it with a simple tool like RabbitMQ management. Pay attention to data structure of rr_headers it is map[string][]string i.e {"header1": ["val1", "val2"]} .

rustatian commented 2 years ago

Thanks, @wodor , I'll address your note to the proper section.