Fixed invalid expiration date bug in Create/Update Legal Tag API calls
All Submissions:
- [YES] I have added an explanation of what changes in this merge do and why we should include it?
- [YES] I have updated the documentation accordingly.
- [YES] I have added tests to cover my changes.
- [YES] All new and existing tests passed.
- [YES] My code follows the code style of this project.
- [YES] I ran lint checks locally prior to submission.
What is the current behavior?
When given a long number for ExpirationDate e.g. 5555555555555555 deserializer converts it in a date (178018-07-19) with 6 digit year value. Although Legal tags get created with this date value but Get call later fails because the method expects the date to be in the "yyyy-mm-dd" format. Please refer the link below for details of issue.
What is the new behavior?
Added method to validate length of year part in deserialized expiration date passed for create/update legal tag. If the year length is not equal to 4 request fails with code 400 and Message ""Expiration Date has invalid year <year part>."
Annotation to deserialize the date was missing in model used for Update Legal Tag. so added that as well.
Added unit test to test the expected behavior if a large numeric value is passed for ExpirationDate.
Added unit test for model UpdateLegalTag to test ExpirationDateDeserializer is correctly deserializing the expiration date and @JsonFormat annotation correctly formats the expiration date during deserialization.
Does this introduce a breaking change?
- [YES]
Any relevant logs, error output, etc?
(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)
Other information
Merge request reports
Activity
added IssueDefect MRBugfix labels
assigned to @ashutoshrai
- Resolved by Ashutosh Rai
I have a few questions about this work, it's not clear to me what the issue we're solving is and whether this is the right way to do it.
First, is there more context on the linked issue? Why does the sample input cause failures? The serialization seems to work initially. Is it because the date provided has a year with more than four digits which fails serialization later on? Additionally, what inputs are actually causing the error? The issue gives two different numbers, and using an online unix timestamp converter neither of them seem to yield the date 178018-07-19. Either way we should probably have a test added with an input that generates this date so we can not only verify we've fixed the bug, but also prevent regressions in the future. Alternatively, maybe this is something that should be fixed with the serializer that we're using so it can properly handle such dates.
Second, the MR description says this is not a breaking change, but I think it should be treated as one. The service will now reject requests with dates formatted as unix timestamps, and based on the linked issue it sounds like this is something that customers are already doing. Would it be better to instead add validation of the date value rather than changing the API contract?
Edited by William Lyles - Last reply by Ashutosh Rai
- Resolved by Ashutosh Rai
Why is test coveraged marked as NA? Wonder if there were unit tests to check the date format and validate expected outcome, the bug could have been avoided. Is it worth adding tests for this case?
- Last reply by Ashutosh Rai
- Resolved by Ashutosh Rai
I’d like to see some additional unit and/or integration tests
- Last reply by Ashutosh Rai
- Resolved by Ashutosh Rai
In general, I wonder if this is the right approach for fixing this issue. Although very not likely, there's nothing inherently wrong with having a timestamp that goes into the future to years greater than 9999, but the possibility of that being a legitimate timestamp is very low.
I would prefer the fix to be to accept years as long as the number of digits is at least 4, rather than requiring exactly 4, but this would mean completely re-doing this MR for something that is extremely unlikely to occur in production workloads.
- Last reply by Ashutosh Rai
reset approvals from @Rustam_Lotsmanenko, @hutchins, @marijadukic, and @dhudsons by pushing to the branch
added 3 commits
-
d73fcc98...48911387 - 2 commits from branch
master - cb4627be - merged with master
-
d73fcc98...48911387 - 2 commits from branch
reset approvals from @hutchins by pushing to the branch
mentioned in commit 6e61016a
