Skip to content

guard against non-hash arg input#6903

Merged
jskinne3 merged 8 commits intomainfrom
jskinne3-json-nil-guard
Sep 1, 2022
Merged

guard against non-hash arg input#6903
jskinne3 merged 8 commits intomainfrom
jskinne3-json-nil-guard

Conversation

@jskinne3
Copy link
Contributor

@jskinne3 jskinne3 commented Sep 1, 2022

addressing undefined method 'slice' for nil:NilClass error in log redacter

Error observed in NewRelic

changelog: Internal, ThreatMetrix API, redacted logging tests

@@ -125,6 +125,8 @@ class ResponseRedacter

# @param [Hash] body
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update the parameter type if we're expecting nil or other non-hash values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm only expecting those values in error situations; they are not the way that the method is intended to be used. So, I would say no, non-hash values should not be documented because they're not supposed to be used. But, I could see the argument the other way, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think an array would be a not-expected value, but I do think nil is sort of expected, so I'd at least document that

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we're handling those other value types in this method, they are supported types for the method. And if we don't want them to be supported types, we shouldn't call the method with those values.

Comment on lines +35 to +37
it 'redacts unknown keys' do
expect(json.values).to eq(['safe value']*3)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

same, I think the whole body would be easier to see

Suggested change
it 'redacts unknown keys' do
expect(json.values).to eq(['safe value']*3)
end
it 'allows known keys' do
expect(json).to eq(
'review_status' => 'safe value',
'summary_risk_score' => 'safe value',
'fraudpoint.score' => 'safe value'
)
end

and maybe even combining this spec with the above? One body that has both known + unknown keys?

jskinne3 and others added 2 commits September 1, 2022 17:31
* Add regression spec for redaction in ResolutionProofingJob

* Add constructor to clarify nil response_body
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM!

@jskinne3 jskinne3 merged commit 8daf6bf into main Sep 1, 2022
@jskinne3 jskinne3 deleted the jskinne3-json-nil-guard branch September 1, 2022 23:19
end
end

context 'empty hash agrument' do
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am unsuccessful in reopening this PR

Copy link
Contributor

@stevegsa stevegsa left a comment

Choose a reason for hiding this comment

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

Looks great!

@zachmargolis zachmargolis mentioned this pull request Sep 7, 2022
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