Expose an errors property for use by consumers #54

aspensmonster commented 2023年08月16日 11:52:13 +02:00

This is a potential solution to the problem found in

keyoxide/keyoxide-web#166

This is a potential solution to the problem found in https://codeberg.org/keyoxide/keyoxide-web/issues/166

aspensmonster commented 2023年09月11日 03:37:19 +02:00

I've updated this pull request to expose an errors property for use by consumers. I presently am using this in some code I'm working on to display errors on the front-end (see my dev-2023-08 branch of my keyoxide-web fork). Some examples of the behaviour can be seen here:

https://findmethere.org/68F640A166E2AD7BD22647B75E1A61D9DC1E1C73

The main change that might be breaking here is that, rather than throwing on invalid URIs or fingerprints, we are now setting an enum status, and consumers need to validate() the claims.

I've updated this pull request to expose an `errors` property for use by consumers. I presently am using this in some code I'm working on to display errors on the front-end (see my `dev-2023-08` branch of my `keyoxide-web` fork). Some examples of the behaviour can be seen here: https://findmethere.org/68F640A166E2AD7BD22647B75E1A61D9DC1E1C73 The main change that might be breaking here is that, rather than throwing on invalid URIs or fingerprints, we are now setting an enum status, and consumers need to `validate()` the claims.

aspensmonster commented 2023年09月13日 05:38:30 +02:00

Feeling good enough about this for a first attempt at getting useful errors propagated to the front-end. Example deployed here:

https://findmethere.org/project@keyoxide.org

Feeling good enough about this for a first attempt at getting useful errors propagated to the front-end. Example deployed here: https://findmethere.org/project@keyoxide.org

yarmo commented 2023年09月14日 18:02:49 +02:00

First impression: this is amazing. All the way hidden in the source code of keyoxide-web is a piece of commented-out template that shows the error messages per claim. At the time, I was unsure how to tackle the issue exactly and, well, 2 years later, there are still no error messages.

A quick dump of thoughts.

Adding _errors to a Claim object

Overall, I like the idea of adding an _errors field to a claim. This is indeed breaking and will need some changes to the Keyoxide spec but that can be arranged.

Claim validation

I like the idea of a validate function. I do wonder if the validation step should not be made mandatory prior to matching? The same way doip-js refuses to verify a claim if it hasn't been matched yet.

Or is this a good moment to rethink the "multiple steps" approach? Obviously, multiple steps (validation, matching, verification) aren't strictly necessary but this decision was made so that UIs could be updated while the time-consuming verification step was happening. Such a rework would fall outside of the scope of this PR but it would be handy to have a clear idea on the matter before merging.

Localized error messages

I think one more issue I have with this approach, and is the biggest issue that stopped me from implementing any error message display: if the library generates them, they will be in English. But the UI that uses doip-js could be in any language and should thus not make any language assumptions.

A potential solution I did not consider at the time: a function inside doip-js that localizes error messages on demand. In this solution, I imagine custom errors as objects with two properties:

• id: unique to each type of error
• arguments: useful information for the error

We can imagine the error with id malformed_uri and the unique argument :no::good:uri. Giving this custom error to a localizer function (called by the UI) may return:

• The URI ":no::good:uri" was malformed.
• L'URI ":no::good:uri" était malformé.

This would mean the library would contain a dictionary with translations. We should strive to making such a function separated through ESM so that tree-shaking would remove it, should the developer not need error localization.

Perhaps localized error messages deserve their own issue/PR. In any case, it would be nice to get a clear picture on the matter before merging.

First impression: this is amazing. All the way hidden in the source code of keyoxide-web is a piece of commented-out template that shows the error messages per claim. At the time, I was unsure how to tackle the issue exactly and, well, 2 years later, there are still no error messages. A quick dump of thoughts. ## Adding _errors to a Claim object Overall, I like the idea of adding an `_errors` field to a claim. This is indeed breaking and will need some changes to the [Keyoxide spec](https://spec.keyoxide.org/spec/2/) but that can be arranged. ## Claim validation I like the idea of a `validate` function. I do wonder if the validation step should not be made mandatory prior to matching? The same way doip-js refuses to verify a claim if it hasn't been matched yet. Or is this a good moment to rethink the "multiple steps" approach? Obviously, multiple steps (validation, matching, verification) aren't strictly necessary but this decision was made so that UIs could be updated while the time-consuming verification step was happening. Such a rework would fall outside of the scope of this PR but it would be handy to have a clear idea on the matter before merging. ## Localized error messages I think one more issue I have with this approach, and is the biggest issue that stopped me from implementing *any* error message display: if the library generates them, they will be in English. But the UI that uses doip-js could be in any language and should thus not make any language assumptions. A potential solution I did not consider at the time: a function inside doip-js that localizes error messages on demand. In this solution, I imagine custom errors as objects with two properties: - id: unique to each type of error - arguments: useful information for the error We can imagine the error with id `malformed_uri` and the unique argument `:no::good:uri`. Giving this custom error to a localizer function (called by the UI) may return: - The URI ":no::good:uri" was malformed. - L'URI ":no::good:uri" était malformé. This would mean the library would contain a dictionary with translations. We should strive to making such a function separated through ESM so that tree-shaking would remove it, should the developer not need error localization. Perhaps localized error messages deserve their own issue/PR. In any case, it would be nice to get a clear picture on the matter before merging.

aspensmonster commented 2023年09月14日 19:33:35 +02:00

I like the idea of a validate function. I do wonder if the validation step should not be made mandatory prior to matching? The same way doip-js refuses to verify a claim if it hasn't been matched yet.

I struggled with figuring that out myself. Presently, the validation happens within the constructor. Either the claim passes construction, or fails immediately. There's no chance to include any sort of error in the claim (since the Claim object doesn't get created) but the validation is guaranteed to happen.

But this PR's approach entails requiring a call to .validate(). And if that call does not happen, then an invalid URI will likely cause an exception later on in matching. My gut would be to introduce another state for ClaimStatus before INIT, perhaps something like PREFLIGHT with a value of 99 (or 0 or some other value less than 100), that the constructor first sets. Then, at the end of .validate(), set the ClaimStatus to INIT if there are no showstoppers (like invalid URI or fingerprint). The match() function can then check that .validate() has been called by ensuring that the ClaimStatus is not PREFLIGHT, and throwing an exception if it is.

Or is this a good moment to rethink the "multiple steps" approach? Obviously, multiple steps (validation, matching, verification) aren't strictly necessary but this decision was made so that UIs could be updated while the time-consuming verification step was happening. Such a rework would fall outside of the scope of this PR but it would be handy to have a clear idea on the matter before merging.

Overall, I like the idea of segmenting the steps. It makes the problem amenable to state machine modelling and encapsulates different stages of processing. As an example, presently, matching is responsible for some heavy lifting in pairing URIs to ServiceProviders, with its own internal state keeping track of what providers to try. But if the work you mentioned in #44 comes to fruition, where claim URIs can be Data URLs with the ServiceProvider explicitly embedded within them, then the matching segment/step becomes a simple "check the schema of the data for the ServiceProvider," along with (perhaps) a check of whether the given ServiceProvider is valid. And in that case, it's likely that the validate step would see heavier lifting instead, checking that the Data URL is well formed and its data conforms to a schema. But from an outside caller's perspective, all of those shifting implementation details remain encapsulated behind validate -> match -> verify.

if the library generates them, they will be in English. But the UI that uses doip-js could be in any language and should thus not make any language assumptions.

A potential solution I did not consider at the time: a function inside doip-js that localizes error messages on demand. In this solution, I imagine custom errors as objects with two properties

I agree that localization should be supported and like the idea of a standardized error object (it feels similar to compiler error lists like in Rust, which can even come with their own explanations, a la rustc --explain E0106). This first pass has _errors as type any[] precisely because the errors can presently arrive from a couple different places: exceptions thrown, the doipjs library producing its own errors like in this PR, and errors returned from the proxy.

Perhaps localized error messages deserve their own issue/PR. In any case, it would be nice to get a clear picture on the matter before merging.

I think I'd like to take a stab at introducing a more unified error type from the get-go, especially if it'll eventually entail updating the spec with what other implementers "should/must" do.

>I like the idea of a validate function. I do wonder if the validation step should not be made mandatory prior to matching? The same way doip-js refuses to verify a claim if it hasn't been matched yet. I struggled with figuring that out myself. Presently, the validation happens within the constructor. Either the claim passes construction, or fails immediately. There's no chance to include any sort of error in the claim (since the `Claim` object doesn't get created) but the validation is guaranteed to happen. But this PR's approach entails requiring a call to `.validate()`. And if that call does not happen, then an invalid URI will likely cause an exception later on in matching. My gut would be to introduce another state for `ClaimStatus` before `INIT`, perhaps something like `PREFLIGHT` with a value of `99` (or `0` or some other value less than `100`), that the constructor first sets. Then, at the end of `.validate()`, set the `ClaimStatus` to `INIT` if there are no showstoppers (like invalid URI or fingerprint). The `match()` function can then check that `.validate()` has been called by ensuring that the `ClaimStatus` is not `PREFLIGHT`, and throwing an exception if it is. >Or is this a good moment to rethink the "multiple steps" approach? Obviously, multiple steps (validation, matching, verification) aren't strictly necessary but this decision was made so that UIs could be updated while the time-consuming verification step was happening. Such a rework would fall outside of the scope of this PR but it would be handy to have a clear idea on the matter before merging. Overall, I like the idea of segmenting the steps. It makes the problem amenable to state machine modelling and encapsulates different stages of processing. As an example, presently, `matching` is responsible for some heavy lifting in pairing `URI`s to `ServiceProvider`s, with its own internal state keeping track of what providers to try. But if the work you mentioned in #44 comes to fruition, where claim URIs can be Data URLs with the `ServiceProvider` explicitly embedded within them, then the `matching` segment/step becomes a simple "check the schema of the data for the `ServiceProvider`," along with (perhaps) a check of whether the given `ServiceProvider` is valid. And in that case, it's likely that the `validate` step would see heavier lifting instead, checking that the Data URL is well formed and its data conforms to a schema. But from an outside caller's perspective, all of those shifting implementation details remain encapsulated behind `validate -> match -> verify`. >if the library generates them, they will be in English. But the UI that uses doip-js could be in any language and should thus not make any language assumptions. > >A potential solution I did not consider at the time: a function inside doip-js that localizes error messages on demand. In this solution, I imagine custom errors as objects with two properties I agree that localization should be supported and like the idea of a standardized error object (it feels similar to compiler error lists like in Rust, which can even come with their own explanations, a la `rustc --explain E0106`). This first pass has _errors as type `any[]` precisely because the errors can presently arrive from a couple different places: exceptions thrown, the doipjs library producing its own errors like in this PR, and errors returned from the proxy. >Perhaps localized error messages deserve their own issue/PR. In any case, it would be nice to get a clear picture on the matter before merging. I think I'd like to take a stab at introducing a more unified error type from the get-go, especially if it'll eventually entail updating the spec with what other implementers "should/must" do.

aspensmonster commented 2023年09月15日 00:15:19 +02:00

I've made a first attempt at supporting error localization. That being said, I'm definitely pushing at the edges of my understanding of the javascript ecosystem here, so I don't know whether tree-shaking would apply to this particular implementation or not. I have it deployed on

https://findmethere.org/

and, depending on how I set my browser's locale, I get either English or Spanish translations. This deployment of course also uses code updates I made in static-src/ of the keyoxide-web project, which can be seen on my dev-2023-08 branch of my fork.

EDIT: Also, I went ahead with the aforementioned PREFLIGHT logic.

I've made a first attempt at supporting error localization. That being said, I'm definitely pushing at the edges of my understanding of the javascript ecosystem here, so I don't know whether tree-shaking would apply to this particular implementation or not. I have it deployed on https://findmethere.org/ and, depending on how I set my browser's locale, I get either English or Spanish translations. This deployment of course also uses code updates I made in `static-src/` of the `keyoxide-web` project, which can be seen on my `dev-2023-08` branch of my fork. EDIT: Also, I went ahead with the aforementioned `PREFLIGHT` logic.

yarmo commented 2023年09月15日 12:40:09 +02:00

Regarding the validation step: explained as such, the separation of steps is indeed still a valid choice. And a PREFLIGHT state is needed and must be validated before attempting to match.

Regarding localized error messages: glad to hear you agree. I am also working on a simplified UI for the web interface to make it easier to translate, so that will pair nicely with localizeable error messages. I like your overall approach, it'll need to be modified slightly to accept the i18next format which is what I used for the ASP tool and will use here as well, together with https://translate.codeberg.org.

I do believe there's stuff here for 2-3 PRs. At least a validate PR (with placeholder error messages) and an extended error messages PR.

Regarding the validation step: explained as such, the separation of steps is indeed still a valid choice. And a `PREFLIGHT` state is needed and must be validated before attempting to match. Regarding localized error messages: glad to hear you agree. I am also working on a simplified UI for the web interface to make it easier to translate, so that will pair nicely with localizeable error messages. I like your overall approach, it'll need to be modified slightly to accept the `i18next` format which is what I used for the ASP tool and will use here as well, together with https://translate.codeberg.org. I do believe there's stuff here for 2-3 PRs. At least a `validate` PR (with placeholder error messages) and an `extended error messages` PR.

aspensmonster commented 2023年09月15日 21:36:37 +02:00

Roger that. I'll look into i18next as an approach for localization. In the meantime, I'll try and split the validate and extended error messages into separate PRs for review.

Roger that. I'll look into `i18next` as an approach for localization. In the meantime, I'll try and split the `validate` and `extended error messages` into separate PRs for review.

This pull request has changes conflicting with the target branch.