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

fix(replays): Run TrimmingProcessor on replays #3706

Merged
merged 4 commits into from
Jun 10, 2024

Conversation

iker-barriocanal
Copy link
Contributor

Although some fields in replays have max_chars defined, the trimming processor isn't run and thus the limit is not enforced. This PR runs the processor and enforces that limit.

@iker-barriocanal iker-barriocanal requested a review from a team June 7, 2024 11:22
@iker-barriocanal iker-barriocanal self-assigned this Jun 7, 2024
@iker-barriocanal iker-barriocanal requested a review from a team as a code owner June 7, 2024 11:22
#[test]
fn test_maxchars_trimming() {
let json = format!(r#"{{"dist": "{}"}}"#, "0".repeat(100));
let mut replay = Annotated::<Replay>::from_json(json.as_str()).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

test only stresses dist, should it test the other fields too?

Copy link
Member

@cmanallen cmanallen left a comment

Choose a reason for hiding this comment

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

As Bruno said more test coverage is appreciated if necessary.

The max length is defined by the annotation on the fields correct? Does this mean string trimming wasn't working before?

@iker-barriocanal
Copy link
Contributor Author

The max length is defined by the annotation on the fields correct?

Yep, that's correct.

Does this mean string trimming wasn't working before?

Yes, let me know if I should clarify further the PR description.

test only stresses dist, should it test the other fields too?

The test validates that the trimming processor runs on replays, and there are separate tests to validate that the trimming processor trims fields with max_chars, so it's ok to move forward with the current state. We can add more tests if there are known edge cases.

@iker-barriocanal iker-barriocanal enabled auto-merge (squash) June 10, 2024 09:26
@iker-barriocanal iker-barriocanal merged commit 674e025 into master Jun 10, 2024
22 checks passed
@iker-barriocanal iker-barriocanal deleted the iker/fix/replay-trimming branch June 10, 2024 09:32
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