Skip to content
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.

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

Unify request/response types at the FacTec API client #120

Merged
merged 13 commits into from Sep 13, 2021

Conversation

tonycdot
Copy link
Contributor

@tonycdot tonycdot commented Sep 7, 2021

Resolves #70.
To do:

  • remove CommonResponse
  • remove AlreadyEnrolled
  • remove custom error structs and change common one
  • remove error field in all Response structs
  • resolve the TODOs at the documentation of the FaceScanSecurityChecks fields
  • implement Deserialize for FacetecResponse
  • clean up robonode-server
  • test error response without error_message and non-error with it

@MOZGIII
Copy link
Contributor

MOZGIII commented Sep 8, 2021

Removing CommonResponse is reasonable since we don't really use it in our business logic. AlreadyEnrolled too - it was just an unused value.

Is there more work planned here? If so, I suggest creating a TODO list in the PR (i.e. like at #38 and #31).

I recall we discussed things like having a single common type for the errors, and extracting error and success into a common response. Is there a change of plans regarding those?

While reviewing the code, I've noticed some other potential improvements:

  1. We can move FaceScanSecurityChecks and FaceScanResponse types to enrollment3d.rs, since they're not used elsewhere.
  2. It would be nice if we could resolve the TODOs at the documentation of the FaceScanSecurityChecks fields.

@tonycdot
Copy link
Contributor Author

tonycdot commented Sep 9, 2021

It would be nice to have smth like this:

#[derive(Debug, Deserialize)]
#[serde(tag = "error")]
enum FacetecResponse<T> {
    /// Error response variant.
    #[serde(rename = true)]
    Err(ServerError),
    #[serde(rename = false)]
    /// Correct response variant.
    Ok(T),
}

But this serde issue is open since 2017, and this pull request is closed, so I decide to reconsider it a bit.
Field error equals to true seems to be always accompanied by error_message, so we can ignore it entirely.

@MOZGIII
Copy link
Contributor

MOZGIII commented Sep 10, 2021

Field error equals to true seems to be always accompanied by error_message, so we can ignore it entirely.

This is what we observed so far, but I don't think we can prove it's actually the case.

My thinking is this: if our intention is to decide whether it's an error or not is based on the error value - then it is what we have to code. If we encounter a case where error is true and there's no error message - well, that would be a parsing error, and we'll easily spot it. If we do this the other way - that is using the presence of an error message itself as an indicator - we'll be unable to detect the case where the error is true and an error message is absent because the behavior that we'll get instead is failed parsing attempt on T; while it should've been parsed as an error.

My suggestion here would be using the error-value-based parsing, and adding explicit test cases for:

  • empty json - which should result in a json parsing error
  • json with error true and no errorMessage - which should also result in a json parsing error
  • json with error false but with an errorMessage - which should parse as T

We might want to add some subclassification for ResponseBody::Json error - to indicate whether an error was at parsing the helper with an error field, the ServerError or T. If we do, we need to make sure it's properly set in tests.

@tonycdot tonycdot marked this pull request as ready for review September 10, 2021 10:41
@tonycdot tonycdot enabled auto-merge (squash) September 13, 2021 13:08
@tonycdot tonycdot merged commit be1e6d2 into master Sep 13, 2021
@tonycdot tonycdot deleted the unify-facetec-types branch September 13, 2021 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants