Skip to content

Commit

Permalink
Remove ProcfileParsingError::RegexError as a user-facing error (#77)
Browse files Browse the repository at this point in the history
There is currently only one error variant for `ProcfileParsingError`,
an error that only occurs if it was not possible to compile the regex
used to parse a `Procfile`.

This regex is buildpack-supplied, so if such an error occurs, it's always
an internal buildpack error.

However previously a user-facing error message was shown, that told
the user to check their `Procfile` was valid, which has no bearing on
whether the regex will compile.

Rather than just correct the error message, the error has been removed
and instead `.expect()` used, since such regex errors will not happen
in practice, given they will be caught by both the `invalid_regex` Clippy
lint and the buildpack's tests. 

I believe part of the reason for this confusion over error messages is
that the error message handling was split across multiple files due to
the use of `thiserror` meaning that the display representation could
be leant upon, without actually looking closely at what was being
displayed.

As such, I've removed `thiserror`, since I think the error handling is
easier to reason about without it.

Fixes #74.
GUS-W-11318868.
  • Loading branch information
edmorley authored Jun 20, 2022
1 parent 9a9ba97 commit 33234be
Show file tree
Hide file tree
Showing 6 changed files with 36 additions and 55 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## Unreleased

- Remove incorrect error message shown in the case of internal buildpack regex errors ([#77](https://github.com/heroku/procfile-cnb/pull/77)).
- Updated `libcnb` and `libherokubuildpack` from 0.5.0 to 0.7.0 ([#49](https://github.com/heroku/procfile-cnb/pull/49) and [#60](https://github.com/heroku/procfile-cnb/pull/60)).

## 1.0.1
Expand Down
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ libcnb = "0.7.0"
libherokubuildpack = "0.7.0"
linked-hash-map = "0.5.4"
regex = "1.5.6"
thiserror = "1.0.31"

[dev-dependencies]
libcnb-test = "0.3.1"
66 changes: 25 additions & 41 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,11 @@ use crate::launch::ProcfileConversionError;
use crate::procfile::ProcfileParsingError;
use indoc::formatdoc;

#[derive(thiserror::Error, Debug)]
#[derive(Debug)]
pub enum ProcfileBuildpackError {
#[error("Cannot read Procfile contents: {0}")]
CannotReadProcfileContents(std::io::Error),
#[error("Procfile parsing error: {0}")]
ProcfileParsingError(#[from] ProcfileParsingError),
#[error("Procfile conversion error: {0}")]
ProcfileConversionError(#[from] ProcfileConversionError),
ProcfileParsingError(ProcfileParsingError),
ProcfileConversionError(ProcfileConversionError),
}

pub fn error_handler(buildpack_error: ProcfileBuildpackError) -> i32 {
Expand All @@ -22,43 +19,30 @@ pub fn error_handler(buildpack_error: ProcfileBuildpackError) -> i32 {
encoded file and try again.
Underlying cause was: {io_error}
",
io_error = io_error
},
);
}
ProcfileBuildpackError::ProcfileParsingError(parsing_error) => {
libherokubuildpack::log_error(
"Cannot parse Procfile",
formatdoc! {"
Please ensure the Procfile in the root of your application is a readable UTF-8
encoded Procfile as specified on Heroku DevCenter and try again:
https://devcenter.heroku.com/articles/procfile
Underlying cause was: {parsing_error}
",
parsing_error = parsing_error
},
);
}
ProcfileBuildpackError::ProcfileConversionError(conversion_error) => {
libherokubuildpack::log_error(
"Cannot convert Procfile to CNB launch configuration",
formatdoc! {"
This is an unexpected internal error that occurs when a Procfile entry is not
compatible with the CNB launch configuration. At the time of writing, Procfile
process names are a strict subset of CNB launch process names and this should
never happen.
Please report this issue with the details below.
Details: {conversion_error}
",
conversion_error = conversion_error
},
"},
);
}
// There are currently no ways in which parsing can fail, however we will add some in the future:
// https://github.com/heroku/procfile-cnb/issues/73
ProcfileBuildpackError::ProcfileParsingError(parsing_error) => match parsing_error {},
ProcfileBuildpackError::ProcfileConversionError(conversion_error) => match conversion_error
{
ProcfileConversionError::InvalidProcessType(libcnb_error) => {
libherokubuildpack::log_error(
"Cannot convert Procfile to CNB launch configuration",
formatdoc! {"
This is an unexpected internal error that occurs when a Procfile entry is not
compatible with the CNB launch configuration. At the time of writing, Procfile
process names are a strict subset of CNB launch process names and this should
never happen.
Please report this issue with the details below.
Details: {libcnb_error}
"},
);
}
},
}

1
Expand Down
3 changes: 1 addition & 2 deletions src/launch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,8 @@ impl TryFrom<Procfile> for Launch {
}
}

#[derive(thiserror::Error, Debug)]
#[derive(Debug)]
pub enum ProcfileConversionError {
#[error("Incompatible process type")]
InvalidProcessType(libcnb::data::launch::ProcessTypeError),
}

Expand Down
19 changes: 9 additions & 10 deletions src/procfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,14 @@ impl FromStr for Procfile {
type Err = ProcfileParsingError;

fn from_str(procfile_contents: &str) -> Result<Self, Self::Err> {
let re_carriage_return_newline =
Regex::new("\\r\\n?").map_err(ProcfileParsingError::RegexError)?;
let re_multiple_newline =
Regex::new("\\n*\\z").map_err(ProcfileParsingError::RegexError)?;
// Using `.expect()` since these can only fail if we've supplied invalid an invalid regex,
// which would be caught by both the `invalid_regex` Clippy lint and the buildpack's tests.
let re_carriage_return_newline = Regex::new("\\r\\n?").expect("Invalid Procfile regex");
let re_multiple_newline = Regex::new("\\n*\\z").expect("Invalid Procfile regex");

// https://github.com/heroku/codon/blob/2613554383cb298076b4a722f4a1aa982ad757e6/lib/slug_compiler/slug.rb#L538-L545
let re_procfile_entry = Regex::new("^[[:space:]]*([a-zA-Z0-9_-]+):?\\s+(.*)[[:space:]]*")
.map_err(ProcfileParsingError::RegexError)?;
.expect("Invalid Procfile regex");

let procfile_contents = re_carriage_return_newline.replace_all(procfile_contents, "\n");
let procfile_contents = re_multiple_newline.replace(&procfile_contents, "\n");
Expand All @@ -62,11 +62,10 @@ impl FromStr for Procfile {
}
}

#[derive(thiserror::Error, Debug, PartialEq)]
pub enum ProcfileParsingError {
#[error("Regex error: {0}")]
RegexError(#[from] regex::Error),
}
// There are currently no ways in which parsing can fail, however we will add some in the future:
// https://github.com/heroku/procfile-cnb/issues/73
#[derive(Debug, PartialEq)]
pub enum ProcfileParsingError {}

#[cfg(test)]
mod tests {
Expand Down

0 comments on commit 33234be

Please sign in to comment.