feat: update extractAllHeaders func to only use the header section for header extraction#286
Conversation
…r header extraction within the implementation, we are first cloning the rawMessage that is provided because the rawMessage and rawHeaderMessage would other wise share the same memory address, afterwards we utilize the strings.Index to find at which point the character occur in this scneario "\r\n\r\n" or "\n\n", and then we update the rawHeaderMessage, we default the entire rawMessage just in case
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the extractAllHeaders function in internal/delivery/parser/parser.go to isolate the header portion of a raw message before parsing. The review feedback points out several critical issues in this implementation, including a duplicate condition bug checking for \r\n\r\n twice, inefficient string cloning, an incorrect index check that misses index 0, and an unnecessary semicolon. A refactored suggestion is provided to address these concerns.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
There's no test exercising the new code path. The existing |
|
Hi @prdai , just a gentle reminder regarding this PR. Could you please take a look at the requested changes/comments when you have some time? Let us know if you need any clarification or assistance. Thanks again for the contribution. |
|
hi, i have been a bit pre occupied with starting work and i am a bit sick these days, could you give me till wed to finish this off? |
Sure, no problem at all. I just wanted to check whether this PR is active or inactive. Take your time and get well soon. |
The header-section-only refactor truncates the raw message at the header/body boundary, which removes the trailing blank line the parse loop relied on to flush the last header. As a result the final header was silently dropped. Flush the pending header after the loop instead. Add tests covering body exclusion for both \r\n\r\n and \n\n separators, and a regression test asserting the first-blank-line break still stops header parsing for mixed line endings (header-injection guard).
Aravinda-HWK
left a comment
Aravinda-HWK
left a comment
There was a problem hiding this comment.
LGTM.
Thanks for the contribution, and we are looking forward to seeing new contributions from you.
3 participants
Description
Updated the Function
extractAllHeaderswithininternal/delivery/parser/parser.go, to first split the rawMessage utilizing double new lines and trying to find the headers within that section instead of going through the entire email message.extractAllHeadersMethod Allocates Entire Email into Memory instead of Header Section Only #279Verified through
make test