Skip to content

fix(types): Serialize BadRequest::FieldViolation reason and localized_message#2461

Open
MathieuTricoire wants to merge 4 commits intohyperium:v0.14.xfrom
MathieuTricoire:field-violation-serialization-fix
Open

fix(types): Serialize BadRequest::FieldViolation reason and localized_message#2461
MathieuTricoire wants to merge 4 commits intohyperium:v0.14.xfrom
MathieuTricoire:field-violation-serialization-fix

Conversation

@MathieuTricoire
Copy link
Copy Markdown

@MathieuTricoire MathieuTricoire commented Dec 5, 2025

Motivation

fix #2460

Fields reason and localized_message from BadRequest::FieldViolation are not serialized into the underlying protobuf message and therefore never make it onto the wire.

Solution

Fix the conversion from FieldViolation to the prost-generated pb::bad_request::FieldViolation by explicitly forwarding both the reason and localized_message fields instead of leaving them at their default values.

A new test is added to ensure that behavior.

}

#[test]
fn gen_bad_request_with_verbose_field_violation() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd recommend either merging the business logic into the original test or creating an incremental fine grained test that tests just the new business logic that has been added in this change.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks @sauravzg for the review! I've merged the new coverage into the original test as suggested. Let me know if this looks good to you.

@MathieuTricoire
Copy link
Copy Markdown
Author

Hi! Just checking in, it looks like a new release went out without the fix. Any chance it could be merged soon?

@MathieuTricoire
Copy link
Copy Markdown
Author

@sauravzg Sorry to ask again but is there any chance it could be merged?

@sauravzg
Copy link
Copy Markdown
Collaborator

@LucioFranco LGTM from my side. Please see if this looks okay and merge.

@MathieuTricoire MathieuTricoire force-pushed the field-violation-serialization-fix branch from 0d75d59 to c3f0aa0 Compare March 31, 2026 16:03
@MathieuTricoire
Copy link
Copy Markdown
Author

MathieuTricoire commented Mar 31, 2026

@LucioFranco @sauravzg I rebased on top of the latest main, would appreciate a fresh approval if needed (no changes from last approval) 🙏

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.

2 participants