Skip to content

WPJ related error codes #2486

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
+20 −0
Open

WPJ related error codes #2486

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d3e6bde
More WPJ Client Exception codes
melissaahn Aug 24, 2024
62937cd
added another error code
melissaahn Aug 26, 2024
f9b5003
dev
melissaahn Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions common4j/src/main/com/microsoft/identity/common/java/exception/ClientException.java
View file
Original file line number Diff line number Diff line change
@@ -87,6 +87,11 @@ public class ClientException extends BaseException {
*/
public static final String DEVICE_TOKEN_EMPTY_OR_NULL = "device_token_empty_or_null";

/**
* Error occurred while requesting a device token.
*/
public static final String DEVICE_TOKEN_REQUEST_FAILED = "device_token_request_failed";

/**
* The sdk failed to parse the Json format.
*/
@@ -424,6 +429,21 @@ public class ClientException extends BaseException {
*/
public static final String WORKPLACE_JOIN_DATA_NULL = "workplace_join_data_null";

/**
* An error occurred related to device registration.
*/
public static final String WORKPLACE_JOIN_DEVICE_REGISTRATION_ERROR = "workplace_join_device_registration_error";

/**
* An error occurred related to device attribute patching.
*/
public static final String WORKPLACE_JOIN_DEVICE_ATTRIBUTE_PATCHING_ERROR = "workplace_join_device_attribute_patching_error";

/**
* An error occurred related to getting device state.
*/
public static final String WORKPLACE_JOIN_DEVICE_STATE_ERROR = "workplace_join_device_state_error";
Comment on lines +432 to +445
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These seem to generic. They just seem to indicate that something went wrong but don't tell exactly what went wrong. Will these be actionable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of these (WORKPLACE_JOIN_DEVICE_REGISTRATION_ERROR) is involved with the direct catching of a WorkplaceJoinException which was in a sort of catch-all block. Further figuring out why this WorkplaceJoinException is being thrown is possible if there's already a throwable attached to it (we could maybe run the ExceptionAdapter on it?), so let me know if that's preferable....
Other cases involve replacing instances where catch blocks create ClientException with UNKNOWN_ERROR directly for unmodeled errors. My understanding of the intention of this was to not lose the location where the original exception gets caught (I think in telemetry, the "location" is where the ClientException got created, and the cause location is where the throwable exception got created). By using the ExceptionAdapter method, I think that "location" value becomes somewhat useless, since it'll lead back to ExceptionAdapter.
I was thinking that since we were considering turning WPJExceptions into a BaseException, which means WPJ might be a big enough feature to warrant throwing the exception directly, it might be helpful to keep that location information. I could be wrong though.
I also understand that it could be weird to have instances of ClientExceptions with a null pointer error code, and others with a wpj error code but a NPE attached.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, it would be good to be able to distinguish one particular WORKPLACE_JOIN_DEVICE_REGISTRATION_ERROR vs another. It can be done by either having a different location/cause or a different error code altogether. I think if there are very few instances then cause will suffice but if there are lots of different instances then we may consider having dedicated errors codes to track these. I think for now what you've done is fine and we should iterate again based on learnings we get from telemetry once this ships.


/**
* Error code to be returned when the broker determines that only account manager can be used
* in the Broker Discovery process.