feat(core): check on load for valid kmx_plus unicode 🙀 by srl295 · Pull Request #13486 · keymanapp/keyman

srl295 · 2025-03-12T23:10:46Z

error out if failure
includes tests
also, improve validation within kmxplus, we were not propagating all validation errors early enough. The implication of this is that transforms might for example appear to just be 'missing' rather than failing out with a validation error. There was not a stability risk that I could see, but we could have had a corrupted keyboard that lacked functions rather than refusing to load.
update docs around kmxplus validity

Fixes: #9446

@keymanapp-test-bot skip

- error out if failure Fixes: #9446

keymanapp-test-bot · 2025-03-12T23:10:50Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

…46-detect-bad-unicode-kmxplus

Fixes: #9446

- turn off some asserts- makes untestable (there are asserts at 'higher levels' such as loading the entire kmx+) - add a test with a synthesized COMP_KMXPLUS_STRS - a valid and an invalid one Fixes: #9446

- not counting markers, which have a special path Fixes: #9446

core/src/ldml/ldml_processor.cpp

srl295 · 2025-03-14T02:51:56Z

Not FFFD?

- add test case - turn this into not be an assert, so we can test it Fixes: #9446

srl295 · 2025-03-14T15:21:45Z

well, this actually opened a bit of a can of worms around is-valid but I'm working on it!

- improve how validation works - a missing section does not mean an invalid section. distingush these. - propagate errors for invalid sections - update documentation of required sections Fixes: #9446

- reinstate 'most or all sections' note - reflow lines Fixes: #9446

srl295 · 2025-03-14T20:43:32Z

@mcdurdin I ended up not being able to test without some updates this given the state of validation propagation! It would actually be more challenging to split this PR, so for review just review the additional commits since you last looked at it.

mcdurdin

I have made some suggestions but they are really nits and polish, so this LGTM. Good work, nice to see robust error handling

core/src/kmx/kmx_plus.cpp

core/src/ldml/ldml_processor.cpp

@mcdurdin

- improve an output string - catch BMP noncharacters besides U+FFFF (H/T @mcdurdin - this was the whole point of the PR) plus test Fixes: #9446 Co-authored-by: Marc Durdin <marc@durdin.net>

- restructured validation calls to be more consistent Fixes: #9446

mcdurdin

I have made some suggestions but they are really nits and polish

I guess one of the suggestions was a bit more significant than a nit, glad to see it has landed!

core/tests/unit/ldml/kmx_plus.tests.cpp

core/src/ldml/ldml_processor.cpp

core/src/kmx/kmx_plus.cpp

- update per review notes Fixes: #9446

keyman-server · 2025-03-19T18:01:10Z

Changes in this pull request will be available for download in Keyman version 19.0.16-alpha

feat(core): check on load for valid kmx_plus unicode

2c2bfe7

- error out if failure Fixes: #9446

github-project-automation bot added this to Keyman Mar 12, 2025

keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Mar 12, 2025

keymanapp-test-bot bot added this to the B18S3 milestone Mar 12, 2025

github-actions bot added core/ Keyman Core feat labels Mar 12, 2025

keymanapp-test-bot bot removed the user-test-missing User tests have not yet been defined for the PR label Mar 12, 2025

Merge branch 'chore/core/9446-kmx-plus-tests-gtest' into feat/core/94…

05f3e3c

…46-detect-bad-unicode-kmxplus

github-actions bot added the common/ label Mar 13, 2025

srl295 changed the base branch from master to chore/core/9446-kmx-plus-tests-gtest March 13, 2025 20:08

srl295 changed the title ~~feat(core): check on load for valid kmx_plus unicode~~ feat(core): check on load for valid kmx_plus unicode 🙀 Mar 13, 2025

feat(core): tests for check on load for valid kmx_plus unicode

fba5f72

Fixes: #9446

github-actions bot added common/ and removed common/ labels Mar 13, 2025

feat(core): tests for check on load for valid kmx_plus unicode

1132fe5

- turn off some asserts- makes untestable (there are asserts at 'higher levels' such as loading the entire kmx+) - add a test with a synthesized COMP_KMXPLUS_STRS - a valid and an invalid one Fixes: #9446

github-actions bot added common/ and removed common/ labels Mar 13, 2025

srl295 marked this pull request as ready for review March 13, 2025 22:18

srl295 requested review from mcdurdin and rc-swag as code owners March 13, 2025 22:18

srl295 requested a review from markcsinclair March 13, 2025 22:18

feat(core): assert that no noncharacter emitted from ldml_processor

d9602ce

- not counting markers, which have a special path Fixes: #9446

github-actions bot added common/ and removed common/ labels Mar 13, 2025

mcdurdin approved these changes Mar 13, 2025

View reviewed changes

core/src/ldml/ldml_processor.cpp Show resolved Hide resolved

Base automatically changed from chore/core/9446-kmx-plus-tests-gtest to master March 14, 2025 14:18

Merge branch 'master' into feat/core/9446-detect-bad-unicode-kmxplus

2e50282

github-actions bot removed the common/ label Mar 14, 2025

feat(core): check non-extended keys for bad unicode

cd00244

- add test case - turn this into not be an assert, so we can test it Fixes: #9446

feat(core,docs): improve kmxplus validation

f1b3555

- improve how validation works - a missing section does not mean an invalid section. distingush these. - propagate errors for invalid sections - update documentation of required sections Fixes: #9446

srl295 requested a review from jahorton as a code owner March 14, 2025 20:37

github-actions bot added docs common/ and removed common/ labels Mar 14, 2025

Merge branch 'master' into feat/core/9446-detect-bad-unicode-kmxplus

45c941f

github-actions bot added common/ and removed common/ labels Mar 14, 2025

feat(docs): update docs on kmxplus

ae13b11

- reinstate 'most or all sections' note - reflow lines Fixes: #9446

github-actions bot added common/ and removed common/ labels Mar 14, 2025

srl295 requested a review from mcdurdin March 14, 2025 20:40

darcywong00 modified the milestones: B18S3, B18S4 Mar 14, 2025

mcdurdin approved these changes Mar 17, 2025

View reviewed changes

srl295 and others added 2 commits March 17, 2025 11:26

feat(core): improve kmxplus validation

0011a2d

- improve an output string - catch BMP noncharacters besides U+FFFF (H/T @mcdurdin - this was the whole point of the PR) plus test Fixes: #9446 Co-authored-by: Marc Durdin <marc@durdin.net>

feat(core): improve kmxplus validation

9989217

- restructured validation calls to be more consistent Fixes: #9446

github-actions bot added common/ and removed common/ labels Mar 17, 2025

mcdurdin approved these changes Mar 18, 2025

View reviewed changes

core/tests/unit/ldml/kmx_plus.tests.cpp Outdated Show resolved Hide resolved

core/src/ldml/ldml_processor.cpp Show resolved Hide resolved

core/src/kmx/kmx_plus.cpp Outdated Show resolved Hide resolved

feat(core): improve kmxplus validation tests

4436819

- update per review notes Fixes: #9446

github-actions bot added common/ and removed common/ labels Mar 18, 2025

srl295 merged commit 4c36f07 into master Mar 19, 2025
20 checks passed

srl295 deleted the feat/core/9446-detect-bad-unicode-kmxplus branch March 19, 2025 15:22

github-project-automation bot moved this to Done in Keyman Mar 19, 2025

Uh oh!

Conversation

srl295 commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keymanapp-test-bot bot commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Test Artifacts

Uh oh!

Uh oh!

srl295 commented Mar 14, 2025 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srl295 commented Mar 14, 2025

Uh oh!

srl295 commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcdurdin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcdurdin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keyman-server commented Mar 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

srl295 commented Mar 12, 2025 •

edited

Loading

keymanapp-test-bot bot commented Mar 12, 2025 •

edited

Loading

srl295 commented Mar 14, 2025 via email •

edited

Loading

srl295 commented Mar 14, 2025 •

edited

Loading