enable to overwrite default headers by command line options

[lyra95]

I would like to use "Content-Type: application/octet-stream" (as it is in https://github.com/google/protorpc/blob/95849c9a3e414b9ba2d3da91fd850156348fe558/protorpc/protobuf.py#L50), but currently it is not possible because content-type header is defined in default headers ("Content-Type: application/x-protobuf") and not overwritten-able.

My use case is:

protocurl -u https://example.com/foo \
	-I ./proto -i ..ReqSomething -o ..ResSomething \
	-d 'something:"1"' --curl --protoc -v \
	-H "Content-Type: application/octet-stream" \
	-H "Accept: application/octet-stream"

[GollyTicker]

Hey, thanks for your interest in contribution! Happy to get in touch with the users of this project.

I can understand the need for this change.

However, the minor caveat is, that your current code is a breaking change, as your provided example command will have different behavior before and after the change.

To make it non-breaking, we could introduce a new flag -n / --no-default-content-type which avoids the content-type being automatically set to the hard-coded default value. However, this solution isn't as easy to type as yours.

Another alternative would introducing a flag -c / --content-type <content-type>. This would overrwide the currently implemented default. I like this more, because it's less verbose than both of the previous solutions and also backwards-compatible.

Your request would then look like this:

protocurl -u https://example.com/foo \
	-I ./proto -i ..ReqSomething -o ..ResSomething \
	-d 'something:"1"' --curl --protoc -v \
	--content-type "application/octet-stream" \
	-H "Accept: application/octet-stream"

What do you think?

Before the merge, we also want to create/adapt the tests to the new functionality.
Would you like to look into the testing documentation and attempt writing new tests?
I'd like to know, whether the docs are sufficently explanatory :)

• decide externally facing API
• review implementation
• create + adapt tests
• release

[lyra95]

Thank you for your suggestion :)
I feel -n / --no-default-content-type flag is much more consistent. (--content-type looks like making special treatment on the header.)

I'll come up with new implementation (including tests) soon

[lyra95]

I introduced no-default-headers flag instead, as you might add more default headers in the future.
I've also included a test but I'm not sure how to run tests locally.

[GollyTicker]

Great! Thanks for updating the code. Your suggestion indeed makes more sense.

You can find info on the tests here. You should also be able to run the tests on your own fork at Actions tab.

I see, that the instruction to run the tests, namely first (once) ./release/10-get-protoc-binaries.sh and then ./test/suite/test.sh $PWD is missing.

• I'll add it soonish then Edit: It was actually already mentioned in the Development section in the README.

I'll go over the code and provide some feedback.

Try to make the tests work locally. You should get the same errors as here.

[lyra95]

I've amended test and docs so now it passes CI.

[GollyTicker]

Thanks for adapting the tests. Unfortunately, it seems like I hdan't submitted the review comments, that I wrote yesterday.

Please take a look at that.

Other than that. I'm happy, that the testing documentation was good enough for you to use it.

[GollyTicker]

Thanks for the quick improvements! Happy to integrate and review your PR :)

One last thing. I think adding this feature to the small set of examples would be great. I think many people actually want to use it, so it makes sense, have this as an explicit example.

You can take a look at this script and the examples template to add the new example. This explains how to run it.

While there is only limited documentation there, I think the existing code should be enough for you to get the right pattern and add the new example.

You could title it something like "Overriding the default Content-Type header and using custom headers" and give and example like -n -H "Content-Type: application/octet-stream" -H "ProtoCurl: is-great".

After that, I don't see any remaining tasks for you and I'll take care of releasing a new minor version upgrade 👍

[lyra95]

I've added an example use case of --no-default-headers flag. Thank you for your review.

[GollyTicker]

Thanks a lot! I'll merge it and release it.

Thanks for contributing. 🥳 I hope you enjoyed it

[lyra95]

I am using protocurl to identify security vulnerabilities, specifically publicly exposed endpoints, on one of the production servers at my workplace. Couldn't have finished my task without this amazing tool. Thanks you!

[GollyTicker]

Awesome! Really nice to hear that! 😊