Skip to content

Add type validation to non-template .arb file parsing logic #139035

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.

Sign up for GitHub

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

Jump to bottom
Merged
auto-submit merged 3 commits into from
Nov 27, 2023
Merged

Add type validation to non-template .arb file parsing logic #139035

auto-submit merged 3 commits into from
Nov 27, 2023

Conversation

@andrewkolos
Copy link
Contributor

@andrewkolos andrewkolos commented Nov 27, 2023 •
edited
Loading

Resolves #138297.

When reading from a .arb file (which contains localizations of strings in the form of a user-defined JSON string-string map), validate the type of values that we read.

See this comment of mine on #138297 to see how I arrived at this fix.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@andrewkolos andrewkolos added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 27, 2023
@andrewkolos andrewkolos mentioned this pull request Nov 27, 2023
Closed
christopherfujino
christopherfujino reviewed Nov 27, 2023
View reviewed changes
packages/flutter_tools/test/general.shard/localizations/gen_l10n_test.dart Outdated
logger = BufferLogger.test();
});

testWithoutContext('AppResourceBundle throws if file contains non-string value', () {
Copy link
Contributor

@christopherfujino christopherfujino Nov 27, 2023

Choose a reason for hiding this comment

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

nit. could/should this be in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/general.shard/generate_localizations_test.dart ?

Copy link
Contributor Author

@andrewkolos andrewkolos Nov 27, 2023

Choose a reason for hiding this comment

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

I was wondering where the non-integration tests for this stuff was. Fixed.

Copy link
Contributor

@christopherfujino christopherfujino Nov 27, 2023

Choose a reason for hiding this comment

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

your file name was better, FWIW

christopherfujino
christopherfujino approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

nit about which library to put the test, otherwise LGTM

@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 27, 2023
@auto-submit auto-submit bot merged commit 1952a6c into flutter:master Nov 27, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 27, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 27, 2023
@engine-flutter-autoroll
1952a6c Add type validation to non-template .arb file parsing logic (f…
7de875d 
…lutter/flutter#139035)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 28, 2023
@engine-flutter-autoroll
Manual roll Flutter from b5f163a to 6bf3ccd (20 revisions) (#5495)
3c05466 
Manual roll requested by tarrinneal@google.com

flutter/flutter@b5f163a...6bf3ccd

2023-11-27 christopherfujino@gmail.com [flutter_tools] fix instructions to disable CLI animations (flutter/flutter#139094)
2023-11-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9fe149249957 to 900ce09b08b2 (3 revisions) (flutter/flutter#139095)
2023-11-27 jonahwilliams@google.com Wide gamut and platform view integration test. (flutter/flutter#138837)
2023-11-27 tessertaha@gmail.com Add ability to customize `NavigationBar` indicator overlay and fix indicator shape for the overlay (flutter/flutter#138901)
2023-11-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from cc57ace8e2e7 to 9fe149249957 (1 revision) (flutter/flutter#139077)
2023-11-27 andrewrkolos@gmail.com Add type validation to non-template .arb file parsing logic (flutter/flutter#139035)
2023-11-27 barpac02@gmail.com Update Android app project template to apply Kotlin Gradle plugin declaratively (flutter/flutter#139006)
2023-11-27 goderbauer@google.com Prepare for dynamically sized views - pt. 2 (flutter/flutter#139079)
2023-11-27 goderbauer@google.com Prepare for dynamically sized views (flutter/flutter#138565)
2023-11-27 JaspervanRiet@users.noreply.github.com Write tests for API examples of BottomNavigationBar and IconButton (flutter/flutter#138188)
2023-11-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9b610ecda771 to cc57ace8e2e7 (2 revisions) (flutter/flutter#139072)
2023-11-27 stuartmorgan@google.com Give an actionable error message when a Pod requires a higher minimum OS version (flutter/flutter#138097)
2023-11-27 reidbaker@google.com Edge case on flutter/flutter/issues/135402 with test (flutter/flutter#138814)
2023-11-27 engine-flutter-autoroll@skia.org Roll Packages from 97f0a40 to e774e88 (4 revisions) (flutter/flutter#139066)
2023-11-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6f499ecea657 to 9b610ecda771 (1 revision) (flutter/flutter#139063)
2023-11-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 61233cde4bb3 to 6f499ecea657 (2 revisions) (flutter/flutter#139051)
2023-11-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 292a92166a94 to 61233cde4bb3 (1 revision) (flutter/flutter#139041)
2023-11-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from ebebb25aaa46 to 292a92166a94 (3 revisions) (flutter/flutter#139037)
2023-11-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from eb18add9ebc5 to ebebb25aaa46 (2 revisions) (flutter/flutter#139034)
2023-11-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from 77432202f766 to eb18add9ebc5 (1 revision) (flutter/flutter#139007)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC rmistry@google.com,stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
caseycrogers pushed a commit to caseycrogers/flutter that referenced this pull request Dec 29, 2023
@andrewkolos @caseycrogers
Add type validation to non-template .arb file parsing logic (flutter#…
ba9ef40 
…139035)

Resolves flutter#138297. 

When reading from a .arb file (which contains localizations of strings in the form of a user-defined JSON string-string map), validate the type of values that we read.

See [this comment of mine on flutter#138297](flutter#138297 (comment)) to see how I arrived at this fix.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
@engine-flutter-autoroll
1952a6c Add type validation to non-template .arb file parsing logic (f…
b5c6f71 
…lutter/flutter#139035)
@andrewkolos andrewkolos deleted the catch-type-error-in-arb-parsing-logic branch April 29, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@christopherfujino christopherfujino christopherfujino approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[tool_crash] _TypeError: (#0 AppResourceBundle.translationFor (package:flutter_tools/src/localizations/gen_l10n_types.dart:637:70))

2 participants

@andrewkolos @christopherfujino