[header map] remove integer setter for string headers#14773
[header map] remove integer setter for string headers#14773asraa merged 3 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
lgtm modulo some very minor nits. Thanks for doing this.
|
|
||
| #define DEFINE_INLINE_HEADER_NUMERIC_FUNCS(name) \ | ||
| DEFINE_INLINE_HEADER_FUNCS(name) \ | ||
| void set##name(uint64_t value) override { setInline(HeaderHandles::get().name, value); } |
There was a problem hiding this comment.
There is no integer getter. Should there be?
There was a problem hiding this comment.
There never was before -- an integer getter would help in the case I mentioned in the description, but I think it's strictly worse to have.
response_headers>setStatus(other->getStatusValue());
Changing it to
response_headers>setStatus(other->getStatusInt());
would mean there'd be a (string -> int -> string) conversion, compared to the no conversion above.
Signed-off-by: Asra Ali <asraa@google.com>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
RE copying values by converting to/from ints: that makes sense; better to leave as string. I thought it might, though remove redundant code to do the conversion at call-sites, but maybe it's not worth it since it's basically one call to whatever the latest wrapper around strtoll is.
|
@envoyproxy/senior-maintainers |
|
ping @envoyproxy/senior-maintainers |
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
Just one question, otherwise LGTM
| { | ||
| // Set and then append. This mimics how GrpcTimeout is set. | ||
| TestRequestHeaderMapImpl headers; | ||
| headers.setGrpcTimeout(42); | ||
| EXPECT_EQ(headers.getGrpcTimeoutValue(), "42"); | ||
| headers.appendGrpcTimeout("s", ""); | ||
| EXPECT_EQ(headers.getGrpcTimeoutValue(), "42s"); | ||
| } |
There was a problem hiding this comment.
Why remove this? Is this covered by another test?
There was a problem hiding this comment.
Yep, it's behavior is covered by Via:
It was meant to test how grpc-timeout is specifically set here
envoy/source/common/grpc/common.cc
Line 223 in 1d1b708
| { | ||
| // Set and then append. This mimics how GrpcTimeout is set. | ||
| TestRequestHeaderMapImpl headers; | ||
| headers.setGrpcTimeout(42); | ||
| EXPECT_EQ(headers.getGrpcTimeoutValue(), "42"); | ||
| headers.appendGrpcTimeout("s", ""); | ||
| EXPECT_EQ(headers.getGrpcTimeoutValue(), "42s"); | ||
| } |
3 participants
Signed-off-by: Asra Ali asraa@google.com
Commit Message: Splits string/numeric header values so that
set##name(int val)is only generated for integer headers andappend##name(string),setReference##name(string)are only generated for header fields that use strings.Additional Description:
QUESTION:
response_headers>setStatus(std::to_string(enumToInt(Code::OK)));thanresponse_headers->setStatus(integer), partly because sometimes the status is set with a value likeheaders->setStatus(other->getStatusValue()). If so, that would simplify the macros.`Risk Level: Med, touches a lot of code, but should not be that scary.
Testing: Tests pass
Fixes: #8567