Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Decode: validate dates #622

Merged
merged 5 commits into from
Oct 18, 2021
Merged

Conversation

jidicula
Copy link
Contributor

@jidicula jidicula commented Oct 8, 2021

Issue: #613

Constructs a datestring with RFC3339 formatting using the parsed date
values, then attempts to call time.Parse() on it, which returns a
non-nil error if datetime string was invalid or time's parsing of
the string failed.

Fixes go test -tags testsuite -run TestTOMLTest_Invalid_Datetime_ImpossibleDate failure.

@jidicula
Copy link
Contributor Author

jidicula commented Oct 8, 2021

I have some local changes adding similar calls to time.Parse() for parseLocalDate(), parseLocalTime(), and parseLocalDateTime() but I'm struggling a bit on where to add tests that adequately cover these changes – should I just be adding cases in TestUnmarshalDecodeErrors()?

@jidicula
Copy link
Contributor Author

jidicula commented Oct 10, 2021

@pelletier how should I add test cases for covering 9d45398? I tried adding an additional case in TestUnmarshalDecodeErrors but adding the case

{
	desc: `invalid local date-time`,
	data: `A = "2021-03-40T23:59:00"`,
	msg:  `invalid local date-time`,
},

doesn't seem to return the expected error.

@pelletier
Copy link
Owner

Sorry for the late reply! TestUnmarshalDecodeErrors should be a good place to put those tests. What error are you getting instead?

Commenting on the general approach taken: this looks like a very expensive approach. Given we have year/month/day already parsed, can we just have a validation step that does not perform allocations?

@jidicula
Copy link
Contributor Author

Sorry for the late reply! TestUnmarshalDecodeErrors should be a good place to put those tests. What error are you getting instead?

--- FAIL: TestUnmarshalDecodeErrors (0.00s)
    --- FAIL: TestUnmarshalDecodeErrors/invalid_local_date-time (0.00s)
        unmarshaler_test.go:2003: 
            	Error Trace:	unmarshaler_test.go:2003
            	Error:      	An error is expected but got nil.
            	Test:       	TestUnmarshalDecodeErrors/invalid_local_date-time
FAIL
exit status 1
FAIL	github.com/pelletier/go-toml/v2	0.016s

As I understand it, this error tester is expecting an error to be returned, but one isn't being returned even though I'm feeding it a test case with an invalid date.

@pelletier
Copy link
Owner

Given the test makes a call to Unmarshal it means that the code path is not hit or the error is lost somewhere.

@jidicula
Copy link
Contributor Author

Commenting on the general approach taken: this looks like a very expensive approach. Given we have year/month/day already parsed, can we just have a validation step that does not perform allocations?

I was wondering about that, it seemed to me that validating day counts for each month reinvents the wheel a bit. It's definitely easier for time and month validation, though. What approach do you suggest for validating days – perhaps a map that maps month-number to its day count? Or, as I'm typing this I'm thinking this might be an appropriate use for a switch statement.

@pelletier
Copy link
Owner

pelletier commented Oct 14, 2021

In that kind of scenario I usually defer to the Go stdlib implementation:

https://github.com/golang/go/blob/24e798e2876f05d628f1e9a32ce8c7f4a3ed3268/src/time/format.go#L1272-L1275

https://github.com/golang/go/blob/24e798e2876f05d628f1e9a32ce8c7f4a3ed3268/src/time/time.go#L1002-L1026

They use a simple table lookup. As far as I know those methods are not publicly accessible. We could use unsafe to access them directly, or we could copy that table and couple functions over. In this case I prefer the latter approach because unsafe exposes us to breakage in future versions of Go, and it's not like the number of days per month is about to change any time soon. The drawback to copyying it over is extra 52 bytes in the resulting binary because of that duplicated table, but I think for usages of go-toml it is an acceptable trade-off.

@jidicula
Copy link
Contributor Author

jidicula commented Oct 15, 2021

Woops, looks like there are some new tests upstream for handling what constitutes a valid time. @pelletier do you think it's better to keep changes introduced in #614 (here) where each element of a time datum is checked as it's parsed, or is it better to check the whole time datum with the isValidTime() function I add here?

@pelletier
Copy link
Owner

pelletier commented Oct 15, 2021 via email

@jidicula
Copy link
Contributor Author

I have no preference on the approach. However the existing tests provide more precise error messages to the user, which is a good property to have. As long as validation makes it clear to the user what the problem is, I don’t have a preference.

Ok, in that case I've removed my redundant time validation 🙂

Copies date checking functionality from go.time.go to verify that the
provided date is indeed valid for a given year, accounting for leap
years.

Resolves: part of pelletier#613
@jidicula
Copy link
Contributor Author

Ready for review now

@jidicula
Copy link
Contributor Author

@pelletier how should I add test cases for covering 9d45398? I tried adding an additional case in TestUnmarshalDecodeErrors but adding the case

{
	desc: `invalid local date-time`,
	data: `A = "2021-03-40T23:59:00"`,
	msg:  `invalid local date-time`,
},

doesn't seem to return the expected error.

Also the resolution for this was that I was mistakenly quoting the date value

Copy link
Owner

@pelletier pelletier left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Added two small comments on the change. Seems like the coverage went down, so it should mean that some code paths are not covered by the tests -- we really try to not bring down test coverage percentage. It's not a perfect metric, but I found it to mostly accomplish its goal.

decode.go Outdated Show resolved Hide resolved
decode.go Outdated Show resolved Hide resolved
Simplifies a return and reduces dependency on `time` library.

Resolves: pelletier#622 (comment)
and pelletier#622 (comment)
@jidicula
Copy link
Contributor Author

jidicula commented Oct 17, 2021

Resolved in 602925b – thanks for the feedback, I'm still relatively new to Go so getting these tips is really helping my learning 😁.

re: code coverage, are the testgen cases run in CI too now that you've added the skip statements to the still-breaking cases?

@jidicula
Copy link
Contributor Author

re: code coverage, are the testgen cases run in CI too now that you've added the skip statements to the still-breaking cases?

nvm, I checked and it is. I would guess that the coverage drop is because of the size of the stuff copied over from time. I'll add test cases for those.

Caused a drop in coverage because it added unreachable lines.
@jidicula
Copy link
Contributor Author

Good call with the coverage comment, I looked deeper and found some unreachable code in a redundant date validity check 🙂. Should be good for re-review now @pelletier.

@pelletier pelletier merged commit 76f53c8 into pelletier:v2 Oct 18, 2021
@pelletier
Copy link
Owner

Looking good, merged! Thank you!

@jidicula jidicula deleted the ji/issue-613-impossible-date branch October 18, 2021 00:18
@jidicula
Copy link
Contributor Author

Looking good, merged! Thank you!

My pleasure! If it's not too much trouble, could you put a hacktoberfest-accepted label on this PR? They don't seem to be tracking review acceptance and merges correctly.

@pelletier
Copy link
Owner

No problem, done. I bet they only look at PRs merged in the main branch, which v2 is not quite ready for that yet.

@jidicula
Copy link
Contributor Author

No problem, done. I bet they only look at PRs merged in the main branch, which v2 is not quite ready for that yet.

Ah, good point. Thanks!

@pelletier pelletier changed the title fix(date test): Fail with invalid date Decode: validate dates Oct 28, 2021
@pelletier pelletier added the bug Issues describing a bug in go-toml. label Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues describing a bug in go-toml. hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants