add http status in vaadinConnectError when accessing service is denied

[ptdatkhtn]

add http status in vaadinConnectError when accessing service is denied

---

This change is 

[manolo]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @manolo and @ptdatkhtn)

---

flow-client/src/main/resources/META-INF/resources/frontend/Connect.ts, line 52 at r1 (raw file):

      throwConnectException(errorJson);
    } else if (errorText !== null && errorText.length > 0) {
      throw new VaadinConnectError(errorText, "VaadinConnectError", response.status);

I would add an object to the detail so as error.detail.status === 500 is what the developer needs to check instead of error.detail === 500 it makes the code more readable, and also allows us to add more info into the detail in the future

[ptdatkhtn]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @manolo)

---

flow-client/src/main/resources/META-INF/resources/frontend/Connect.ts, line 52 at r1 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

I would add an object to the detail so as error.detail.status === 500 is what the developer needs to check instead of error.detail === 500 it makes the code more readable, and also allows us to add more info into the detail in the future

Done.

[manolo]

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @manolo and @ptdatkhtn)

---

flow-client/src/main/resources/META-INF/resources/frontend/Connect.ts, line 59 at r2 (raw file):

detail.status

the argument to the exception shuld be the detail

[ptdatkhtn]

---

flow-client/src/main/resources/META-INF/resources/frontend/Connect.ts, line 59 at r2 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

detail.status

the argument to the exception shuld be the detail

thanks, sorry, my mistake

[platosha]

Reviewed 1 of 2 files at r1, 1 of 1 files at r3.
Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @manolo and @ptdatkhtn)

a discussion (no related file):
Missing tests.

Not only code is not tested, but also it is hard to tell what does the usage look like in the review.

---

---

flow-client/src/main/resources/META-INF/resources/frontend/Connect.ts, line 52 at r1 (raw file):

Previously, ptdatkhtn (Thanh Dat PHAN) wrote…

Done.

Honestly, I don’t think it’s a good idea to use detail here, because it’s original purpose is to carry detail property corresponding to VaadinConnectException.java passed from the server. The declared type is any already. So in the end, in the IDE, nothing meaningful will be suggested for detail because of its any.

It would be better to add a direct class property here, like responseStatus or something, or maybe even expose the entire response, see the Response class definition, it has status, statusText, and so on: https://developer.mozilla.org/en-US/docs/Web/API/Response

Here is the catch, though: currently, there are two kinds of things mixed here: errors passed from the server’s VaadinConnectException, and also errors for unexpected HTTP responses. VaadinConnectError is thrown in both cases. Developers might have to determine the reason behind the error in the end to be able to handle it correctly, then mixing two different kinds of errors in the same type would be problematic.

I think it would make sense to untangle them, and introduce a special subtype for response error, which is not related to VaadinConnectException, like VaadinConnectResponseError, so that this error is thrown for unexpected responses, and it has the response property.

Then in the usage it would be easier to determine the reason behind the error, something like that:

try {
  await myEndpoint.myMethod();
} catch(error) {
  if (error instanceof VaadinConnectValidationError) {
    // invalid request data, display error message in the form
  } else if (error instanceof VaadinConnectResponseError) {
    // unexpected response, possibly server is broken
    // maybe report somewhere
    console.error("HTTP error:", error.response.status, error.response.statusText);
  } else if (error instanceof VaadinConnectError) {
    // VaadinConnectException from Java
  }
}

---

flow-client/src/main/resources/META-INF/resources/frontend/Connect.ts, line 58 at r3 (raw file):

      const detail: VaadinConnectErrorDetail = {
        status: response.status
      }

Missing ; in the end of line. The object is an inline statement, so here should be a semicolon. Ideally this should be checked in the linter though...

[platosha]

Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @manolo and @ptdatkhtn)

[manolo]

Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @manolo, @platosha, and @ptdatkhtn)

---

flow-client/src/main/resources/META-INF/resources/frontend/Connect.ts, line 52 at r1 (raw file):

Previously, platosha (Anton Platonov) wrote…

Honestly, I don’t think it’s a good idea to use detail here, because it’s original purpose is to carry detail property corresponding to VaadinConnectException.java passed from the server. The declared type is any already. So in the end, in the IDE, nothing meaningful will be suggested for detail because of its any.

It would be better to add a direct class property here, like responseStatus or something, or maybe even expose the entire response, see the Response class definition, it has status, statusText, and so on: https://developer.mozilla.org/en-US/docs/Web/API/Response

Here is the catch, though: currently, there are two kinds of things mixed here: errors passed from the server’s VaadinConnectException, and also errors for unexpected HTTP responses. VaadinConnectError is thrown in both cases. Developers might have to determine the reason behind the error in the end to be able to handle it correctly, then mixing two different kinds of errors in the same type would be problematic.

I think it would make sense to untangle them, and introduce a special subtype for response error, which is not related to VaadinConnectException, like VaadinConnectResponseError, so that this error is thrown for unexpected responses, and it has the response property.

Then in the usage it would be easier to determine the reason behind the error, something like that:

try {
  await myEndpoint.myMethod();
} catch(error) {
  if (error instanceof VaadinConnectValidationError) {
    // invalid request data, display error message in the form
  } else if (error instanceof VaadinConnectResponseError) {
    // unexpected response, possibly server is broken
    // maybe report somewhere
    console.error("HTTP error:", error.response.status, error.response.statusText);
  } else if (error instanceof VaadinConnectError) {
    // VaadinConnectException from Java
  }
}

👍

[manolo]

Reviewed 1 of 2 files at r1, 1 of 1 files at r3.
Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @platosha and @ptdatkhtn)

[ptdatkhtn]

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @manolo and @platosha)

a discussion (no related file):

Previously, platosha (Anton Platonov) wrote…

Missing tests.

Not only code is not tested, but also it is hard to tell what does the usage look like in the review.

Done.

---

---

flow-client/src/main/resources/META-INF/resources/frontend/Connect.ts, line 58 at r3 (raw file):

Previously, platosha (Anton Platonov) wrote…

Missing ; in the end of line. The object is an inline statement, so here should be a semicolon. Ideally this should be checked in the linter though...

Done.

[platosha]

Reviewed 2 of 2 files at r4.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @platosha)

[platosha]

Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained