enhance: include permission identifier in ACL error messages - #15373
Conversation
Package Changes Through 7d65eadThere are 2 changes which include tauri-utils with patch, tauri-bundler with patch Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
| #[error("identifiers can only include lowercase ASCII, hyphens which are not leading or trailing, and a single colon if using a prefix")] | ||
| InvalidFormat, | ||
| #[error("invalid plugin or permission identifier '{0}': identifiers can only include lowercase ASCII letters, digits, hyphens (not leading or trailing), and a single colon when using a prefix")] | ||
| InvalidFormat(String), |
There was a problem hiding this comment.
This unfortunately is a breaking change and has to wait for v3
…erialize boundary Per @FabianLars feedback on PR tauri-apps#15373: changing InvalidFormat from a unit variant to a tuple variant is a breaking change to ParseIdentifierError and needs to wait for v3. Revert the variant signature and capture the offending identifier value in the Identifier Deserialize impl instead, so the user-visible serde error message still names the bad entry without touching the public enum surface. Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
|
Thanks @FabianLars, I agreed. I just pushed a revision that keeps The three new path-attached parse variants ( |
|
|
||
| /// Invalid TOML encountered | ||
| #[error("failed to parse TOML: {0}")] | ||
| Toml(#[from] toml::de::Error), |
There was a problem hiding this comment.
Let's deprecate the old ones and add a todo for the removal in v3
There was a problem hiding this comment.
Actually.. We didn't mark this error type #[non_exhaustive] so adding a new type is also breaking...
Do you mind splitting the Identifier deserialization code to a new PR so we can merge it first? The error message change will probably need to wait for v3
|
|
||
| /// Invalid TOML encountered while parsing a known file. | ||
| #[error("failed to parse TOML file '{}': {}", _1.display(), _0)] | ||
| TomlFile(toml::de::Error, PathBuf), |
There was a problem hiding this comment.
Could we align this with
e.g.
/// The path that failed to parse into JSON.
path: PathBuf,
/// The parsing [`serde_json::Error`].
error: serde_json::Error,(p.s. not the name, just tuple to struct)
…rialize wrapping Legend-Master noted on PR tauri-apps#15373 that `Error` is not `#[non_exhaustive]`, so adding `TomlFile`/`JsonFile`/`Json5File` variants is itself a breaking change. Drop those variants and revert the build/capability call sites to the existing `Toml`/`Json`/`Json5` variants via `#[from]`. Keep only the `Identifier::Deserialize` wrapping so the offending identifier still appears in the serde error message. File-path attached parse variants can return as struct variants in v3. Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
79e7ab8 to
5c40858
Compare
Legend-Master
left a comment
There was a problem hiding this comment.
Thanks, awesome improvement!
Also you'll need to sign your commits for me to merge this
Adds Error::TomlFile, Error::JsonFile, and Error::Json5File variants that carry the file path. ParseIdentifierError::InvalidFormat now carries the offending value as a String. Together these turn the previous opaque build failure into one that names the file and the bad identifier. Closes tauri-apps#11262 Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
…erialize boundary Per @FabianLars feedback on PR tauri-apps#15373: changing InvalidFormat from a unit variant to a tuple variant is a breaking change to ParseIdentifierError and needs to wait for v3. Revert the variant signature and capture the offending identifier value in the Identifier Deserialize impl instead, so the user-visible serde error message still names the bad entry without touching the public enum surface. Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
…rialize wrapping Legend-Master noted on PR tauri-apps#15373 that `Error` is not `#[non_exhaustive]`, so adding `TomlFile`/`JsonFile`/`Json5File` variants is itself a breaking change. Drop those variants and revert the build/capability call sites to the existing `Toml`/`Json`/`Json5` variants via `#[from]`. Keep only the `Identifier::Deserialize` wrapping so the offending identifier still appears in the serde error message. File-path attached parse variants can return as struct variants in v3. Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
5c40858 to
7d65ead
Compare
What kind of change does this PR introduce?
Enhancement to ACL error diagnostics.
Description
Closes #11262 (level 2 of the three-tier ask: identify the file and clarify
which identifier is invalid).
Errorenum (TomlFile,JsonFile,Json5File) carrying the parse error and the file path. Thelegacy
Toml,Json,Json5variants stay for non-file parse contexts.acl/build.rs(3) andacl/capability.rs(3) to use the new variants via
map_err, so the file path is reported in theerror.
ParseIdentifierError::InvalidFormatfrom a unit variant toInvalidFormat(String)carrying the offending identifier. Updates the messageto say "invalid plugin or permission identifier '': ..." rather than
the previous generic "identifiers can only include ...".
The composite effect on the user-facing error (the exact scenario from
#11262):
Before:
failed to parse JSON: identifiers can only include lowercase ASCII, hyphens which are not leading or trailing, and a single colon if using a prefix at line 16 column 23After:
failed to parse JSON file '/path/to/permissions/default.toml': invalid plugin or permission identifier 'sqlite_proxy:allow-foo': identifiers can only include lowercase ASCII letters, digits, hyphens (not leading or trailing), and a single colon when using a prefix at line 16 column 23Notes for review
InvalidFormatvariant signature change is technically a breaking APIchange. I verified there are zero in-tree pattern matches on
ParseIdentifierErrorvariants (grep -rn 'ParseIdentifierError::' crates/),so no internal callers break. External pattern matchers (if any) would need
to update from
InvalidFormattoInvalidFormat(_). The covector changesetis
minor:enhance; happy to bump tomajor:breakingif you prefer.at docs(plugins): document plugin and permission identifier syntax rules tauri-docs#3826.
serde_untagged(referenced in [feat] Better errors when deserializing capabilities #9951) and is intentionally out of scope here.Build, test, fmt, clippy run locally on
tauri-utils --features build,config-json5; all clean.