Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix panic in log with empty header #51

Merged
merged 1 commit into from
Jun 14, 2022
Merged

Fix panic in log with empty header #51

merged 1 commit into from
Jun 14, 2022

Conversation

arp242
Copy link
Contributor

@arp242 arp242 commented Jun 14, 2022

Previously using l.SetHeader("") would panic with:

--- FAIL: TestEmptyHeader (0.00s)
panic: runtime error: index out of range [-1] [recovered]
	panic: runtime error: index out of range [-1]

goroutine 18 [running]:
github.com/labstack/gommon/log.(*Logger).log(0xc00014e990, 0x1, {0x5fb505, 0xd}, {0x0, 0x0, 0x0})
	/home/martin/src/gommon/log/log.go:394 +0x615
github.com/labstack/gommon/log.(*Logger).Debugf(...)
	/home/martin/src/gommon/log/log.go:158
github.com/labstack/gommon/log.TestEmptyHeader(0xc000129860?)
	/home/martin/src/gommon/log/log_test.go:128 +0xb1

This adds a check for that. It also checks if there is any content
before writing a space in the "Text header" else branch so you don't end
up with " my message".

Previously using `l.SetHeader("")` would panic with:

	--- FAIL: TestEmptyHeader (0.00s)
	panic: runtime error: index out of range [-1] [recovered]
		panic: runtime error: index out of range [-1]

	goroutine 18 [running]:
	github.com/labstack/gommon/log.(*Logger).log(0xc00014e990, 0x1, {0x5fb505, 0xd}, {0x0, 0x0, 0x0})
		/home/martin/src/gommon/log/log.go:394 +0x615
	github.com/labstack/gommon/log.(*Logger).Debugf(...)
		/home/martin/src/gommon/log/log.go:158
	github.com/labstack/gommon/log.TestEmptyHeader(0xc000129860?)
		/home/martin/src/gommon/log/log_test.go:128 +0xb1

This adds a check for that. It also checks if there is any content
before writing a space in the "Text header" else branch so you don't end
up with " my message".
Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@codecov-commenter
Copy link

Codecov Report

Merging #51 (f718471) into master (64116ba) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
+ Coverage   60.64%   60.70%   +0.06%     
==========================================
  Files           6        6              
  Lines         625      626       +1     
==========================================
+ Hits          379      380       +1     
  Misses        242      242              
  Partials        4        4              
Impacted Files Coverage Δ
log/log.go 53.15% <100.00%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64116ba...f718471. Read the comment docs.

@aldas aldas merged commit 6267eb7 into labstack:master Jun 14, 2022
@aldas
Copy link
Contributor

aldas commented Jun 14, 2022

@arp242 Thank you

@arp242 arp242 deleted the empty-header branch June 14, 2022 22:42
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants