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

CORE-15778: add meaningful exception messages in StateRef.parse() #1199

Merged
merged 9 commits into from
Aug 8, 2023

Conversation

jennyang-r3
Copy link
Contributor

@jennyang-r3 jennyang-r3 commented Aug 3, 2023

Overview

The PR

  • added an exception in malformed value in StateRef where delimiter, ":", isn't in value.
  • optimise the method by utilising an existing index exception.

Testing

I added a unit test for StateRef.parse() method, testing if:

  • valid StateRef is parsed correctly
  • value with zero delimiter throws a correct exception
  • value with missing index throws a correct exception
  • any other malformed value throws transaction Id exception

@corda-jenkins-ci02
Copy link
Contributor

corda-jenkins-ci02 bot commented Aug 3, 2023

Jenkins build for PR 1199 build 12

Build Successful:
Jar artifact version produced by this PR: 5.1.0.12-alpha-1691482422220

@jennyang-r3 jennyang-r3 marked this pull request as ready for review August 4, 2023 07:45
@jennyang-r3 jennyang-r3 requested a review from a team as a code owner August 4, 2023 07:45
Copy link
Contributor

@relyafi relyafi left a comment

Choose a reason for hiding this comment

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

I've posted some specific comments, but having looked at the change as a whole, I wonder if there's a simpler solution. We're already parsing the hash, and verifying the index, so could the issue be solved by parsing the index before calling parseSecureHash? The index check could also probably be made a bit stronger by replacing the (now deprecated?) parseInt with toUInt?

I also think it's worth elaborating on the description, and adding test notes - @nkovacsx PR's are often good examples of this, e.g. corda/corda-runtime-os#4230

Finally, it feels like we're currently missing a unit test case if we were able to make this change without breaking any tests - I think it's worth adding a case to check that we get the expected error if a malformed state ref is passed in.

@jennyang-r3 jennyang-r3 marked this pull request as draft August 4, 2023 11:08
@jennyang-r3 jennyang-r3 marked this pull request as ready for review August 7, 2023 15:48
@jennyang-r3 jennyang-r3 requested review from vlajos and removed request for lankydan August 7, 2023 16:54
@jennyang-r3 jennyang-r3 requested a review from vlajos August 8, 2023 08:29
Copy link
Contributor

@vlajos vlajos left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@relyafi relyafi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@driessamyn driessamyn left a comment

Choose a reason for hiding this comment

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

LGTM

@jennyang-r3 jennyang-r3 merged commit ae86383 into release/os/5.1 Aug 8, 2023
1 check passed
@jennyang-r3 jennyang-r3 deleted the jennyang/CORE-15778 branch August 8, 2023 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants