(Unknown)RemoteException implement ResponseStatus by schlosna · Pull Request #1026 · palantir/conjure-java-runtime-api

schlosna · 2023-09-11T16:33:56Z

Before this PR

Consumers that handle UnknownRemoteException and RemoteException via getStatus() often require duplicative code paths because there is not a shared interface to retrieve the HTTP response codes.

After this PR

==COMMIT_MSG==
(Unknown)RemoteException implement ResponseStatus
==COMMIT_MSG==

Possible downsides?

We already expose getStatus() as public API on both exception types, but do we want to expose this interface as super type?

changelog-app · 2023-09-11T16:33:59Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

(Unknown)RemoteException implement ResponseStatus

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2023-09-14T02:11:47Z

+
+public interface ResponseStatus {
+ /** The HTTP status code of the HTTP response conveying the error. */
+ int getStatus();


I don't have a strong opinion on this. Below is a light braindump thinking through it
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
ServiceException is used within a server to produce a specific error, while RemoteException and UnknownRemoteException are thrown by clients to describe an error response. QosException sort of does both.
Would supporting this interface create the impression that it covers all known response-related exceptions?

I'm on the fence here -- I think there are some cases where it is useful to expose the HTTP status code that triggered the RemoteException, though I think for this case "When in doubt, leave it out". I'm leaning toward just closing this PR.
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
The few places I was looking at where this would be useful would be something along the lines of below. I've refactored those places to just have 2 separate catch clauses that call the same method with the status code.
} catch (RemoteException | UnknownRemoteException e) { // logic using e.getStatus()
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
I could see the status for QosException potentially being useful for some diagnostics, but probably not worth it and exposing those implementation details.
Would supporting this interface create the impression that it covers all known response-related exceptions?
I was thinking of this as something along the lines of SQLException#getErrorCode though in this case they are part of HTTP standards in RFC 9110 and not vendor specific.

carterkozak · 2023-09-14T02:13:57Z

- args.add(SafeArg.of("status", getStatus()));
- args.add(UnsafeArg.of("body", getBody()));
- return Collections.unmodifiableList(args);
+ return List.of(SafeArg.of("status", getStatus()), UnsafeArg.of("body", getBody()));


Any chance you could pull the String.format + collection creation improvements into a separate PR? Would love to get those improvements out, and merging doesn't require me to think as hard about api tradeoffs :-)

no problem, split that out into #1027

schlosna · 2023-09-11T16:33:56Z

Before this PR

Consumers that handle UnknownRemoteException and RemoteException via getStatus() often require duplicative code paths because there is not a shared interface to retrieve the HTTP response codes.

After this PR

==COMMIT_MSG==
(Unknown)RemoteException implement ResponseStatus
==COMMIT_MSG==

Possible downsides?

We already expose getStatus() as public API on both exception types, but do we want to expose this interface as super type?

changelog-app · 2023-09-11T16:33:59Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

(Unknown)RemoteException implement ResponseStatus

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2023-09-14T02:11:47Z

+
+public interface ResponseStatus {
+ /** The HTTP status code of the HTTP response conveying the error. */
+ int getStatus();


I don't have a strong opinion on this. Below is a light braindump thinking through it
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
ServiceException is used within a server to produce a specific error, while RemoteException and UnknownRemoteException are thrown by clients to describe an error response. QosException sort of does both.
Would supporting this interface create the impression that it covers all known response-related exceptions?

I'm on the fence here -- I think there are some cases where it is useful to expose the HTTP status code that triggered the RemoteException, though I think for this case "When in doubt, leave it out". I'm leaning toward just closing this PR.
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
The few places I was looking at where this would be useful would be something along the lines of below. I've refactored those places to just have 2 separate catch clauses that call the same method with the status code.
} catch (RemoteException | UnknownRemoteException e) { // logic using e.getStatus()
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
I could see the status for QosException potentially being useful for some diagnostics, but probably not worth it and exposing those implementation details.
Would supporting this interface create the impression that it covers all known response-related exceptions?
I was thinking of this as something along the lines of SQLException#getErrorCode though in this case they are part of HTTP standards in RFC 9110 and not vendor specific.

carterkozak · 2023-09-14T02:13:57Z

- args.add(SafeArg.of("status", getStatus()));
- args.add(UnsafeArg.of("body", getBody()));
- return Collections.unmodifiableList(args);
+ return List.of(SafeArg.of("status", getStatus()), UnsafeArg.of("body", getBody()));


Any chance you could pull the String.format + collection creation improvements into a separate PR? Would love to get those improvements out, and merging doesn't require me to think as hard about api tradeoffs :-)

no problem, split that out into #1027

schlosna · 2023-09-11T16:33:56Z

Before this PR

Consumers that handle UnknownRemoteException and RemoteException via getStatus() often require duplicative code paths because there is not a shared interface to retrieve the HTTP response codes.

After this PR

==COMMIT_MSG==
(Unknown)RemoteException implement ResponseStatus
==COMMIT_MSG==

Possible downsides?

We already expose getStatus() as public API on both exception types, but do we want to expose this interface as super type?

changelog-app · 2023-09-11T16:33:59Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

(Unknown)RemoteException implement ResponseStatus

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2023-09-14T02:11:47Z

+
+public interface ResponseStatus {
+ /** The HTTP status code of the HTTP response conveying the error. */
+ int getStatus();


I don't have a strong opinion on this. Below is a light braindump thinking through it
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
ServiceException is used within a server to produce a specific error, while RemoteException and UnknownRemoteException are thrown by clients to describe an error response. QosException sort of does both.
Would supporting this interface create the impression that it covers all known response-related exceptions?

I'm on the fence here -- I think there are some cases where it is useful to expose the HTTP status code that triggered the RemoteException, though I think for this case "When in doubt, leave it out". I'm leaning toward just closing this PR.
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
The few places I was looking at where this would be useful would be something along the lines of below. I've refactored those places to just have 2 separate catch clauses that call the same method with the status code.
} catch (RemoteException | UnknownRemoteException e) { // logic using e.getStatus()
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
I could see the status for QosException potentially being useful for some diagnostics, but probably not worth it and exposing those implementation details.
Would supporting this interface create the impression that it covers all known response-related exceptions?
I was thinking of this as something along the lines of SQLException#getErrorCode though in this case they are part of HTTP standards in RFC 9110 and not vendor specific.

carterkozak · 2023-09-14T02:13:57Z

- args.add(SafeArg.of("status", getStatus()));
- args.add(UnsafeArg.of("body", getBody()));
- return Collections.unmodifiableList(args);
+ return List.of(SafeArg.of("status", getStatus()), UnsafeArg.of("body", getBody()));


Any chance you could pull the String.format + collection creation improvements into a separate PR? Would love to get those improvements out, and merging doesn't require me to think as hard about api tradeoffs :-)

no problem, split that out into #1027

schlosna · 2023-09-11T16:33:56Z

Before this PR

Consumers that handle UnknownRemoteException and RemoteException via getStatus() often require duplicative code paths because there is not a shared interface to retrieve the HTTP response codes.

After this PR

==COMMIT_MSG==
(Unknown)RemoteException implement ResponseStatus
==COMMIT_MSG==

Possible downsides?

We already expose getStatus() as public API on both exception types, but do we want to expose this interface as super type?

changelog-app · 2023-09-11T16:33:59Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

(Unknown)RemoteException implement ResponseStatus

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2023-09-14T02:11:47Z

+
+public interface ResponseStatus {
+ /** The HTTP status code of the HTTP response conveying the error. */
+ int getStatus();


I don't have a strong opinion on this. Below is a light braindump thinking through it
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
ServiceException is used within a server to produce a specific error, while RemoteException and UnknownRemoteException are thrown by clients to describe an error response. QosException sort of does both.
Would supporting this interface create the impression that it covers all known response-related exceptions?

I'm on the fence here -- I think there are some cases where it is useful to expose the HTTP status code that triggered the RemoteException, though I think for this case "When in doubt, leave it out". I'm leaning toward just closing this PR.
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
The few places I was looking at where this would be useful would be something along the lines of below. I've refactored those places to just have 2 separate catch clauses that call the same method with the status code.
} catch (RemoteException | UnknownRemoteException e) { // logic using e.getStatus()
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
I could see the status for QosException potentially being useful for some diagnostics, but probably not worth it and exposing those implementation details.
Would supporting this interface create the impression that it covers all known response-related exceptions?
I was thinking of this as something along the lines of SQLException#getErrorCode though in this case they are part of HTTP standards in RFC 9110 and not vendor specific.

carterkozak · 2023-09-14T02:13:57Z

- args.add(SafeArg.of("status", getStatus()));
- args.add(UnsafeArg.of("body", getBody()));
- return Collections.unmodifiableList(args);
+ return List.of(SafeArg.of("status", getStatus()), UnsafeArg.of("body", getBody()));


Any chance you could pull the String.format + collection creation improvements into a separate PR? Would love to get those improvements out, and merging doesn't require me to think as hard about api tradeoffs :-)

no problem, split that out into #1027

schlosna · 2023-09-11T16:33:56Z

Before this PR

Consumers that handle UnknownRemoteException and RemoteException via getStatus() often require duplicative code paths because there is not a shared interface to retrieve the HTTP response codes.

After this PR

==COMMIT_MSG==
(Unknown)RemoteException implement ResponseStatus
==COMMIT_MSG==

Possible downsides?

We already expose getStatus() as public API on both exception types, but do we want to expose this interface as super type?

changelog-app · 2023-09-11T16:33:59Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

(Unknown)RemoteException implement ResponseStatus

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2023-09-14T02:11:47Z

+
+public interface ResponseStatus {
+ /** The HTTP status code of the HTTP response conveying the error. */
+ int getStatus();


I don't have a strong opinion on this. Below is a light braindump thinking through it
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
ServiceException is used within a server to produce a specific error, while RemoteException and UnknownRemoteException are thrown by clients to describe an error response. QosException sort of does both.
Would supporting this interface create the impression that it covers all known response-related exceptions?

I'm on the fence here -- I think there are some cases where it is useful to expose the HTTP status code that triggered the RemoteException, though I think for this case "When in doubt, leave it out". I'm leaning toward just closing this PR.
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
The few places I was looking at where this would be useful would be something along the lines of below. I've refactored those places to just have 2 separate catch clauses that call the same method with the status code.
} catch (RemoteException | UnknownRemoteException e) { // logic using e.getStatus()
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
I could see the status for QosException potentially being useful for some diagnostics, but probably not worth it and exposing those implementation details.
Would supporting this interface create the impression that it covers all known response-related exceptions?
I was thinking of this as something along the lines of SQLException#getErrorCode though in this case they are part of HTTP standards in RFC 9110 and not vendor specific.

carterkozak · 2023-09-14T02:13:57Z

- args.add(SafeArg.of("status", getStatus()));
- args.add(UnsafeArg.of("body", getBody()));
- return Collections.unmodifiableList(args);
+ return List.of(SafeArg.of("status", getStatus()), UnsafeArg.of("body", getBody()));


Any chance you could pull the String.format + collection creation improvements into a separate PR? Would love to get those improvements out, and merging doesn't require me to think as hard about api tradeoffs :-)

no problem, split that out into #1027

schlosna · 2023-09-11T16:33:56Z

Before this PR

Consumers that handle UnknownRemoteException and RemoteException via getStatus() often require duplicative code paths because there is not a shared interface to retrieve the HTTP response codes.

After this PR

==COMMIT_MSG==
(Unknown)RemoteException implement ResponseStatus
==COMMIT_MSG==

Possible downsides?

We already expose getStatus() as public API on both exception types, but do we want to expose this interface as super type?

changelog-app · 2023-09-11T16:33:59Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

(Unknown)RemoteException implement ResponseStatus

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2023-09-14T02:11:47Z

+
+public interface ResponseStatus {
+ /** The HTTP status code of the HTTP response conveying the error. */
+ int getStatus();


I don't have a strong opinion on this. Below is a light braindump thinking through it
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
ServiceException is used within a server to produce a specific error, while RemoteException and UnknownRemoteException are thrown by clients to describe an error response. QosException sort of does both.
Would supporting this interface create the impression that it covers all known response-related exceptions?

I'm on the fence here -- I think there are some cases where it is useful to expose the HTTP status code that triggered the RemoteException, though I think for this case "When in doubt, leave it out". I'm leaning toward just closing this PR.
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
The few places I was looking at where this would be useful would be something along the lines of below. I've refactored those places to just have 2 separate catch clauses that call the same method with the status code.
} catch (RemoteException | UnknownRemoteException e) { // logic using e.getStatus()
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
I could see the status for QosException potentially being useful for some diagnostics, but probably not worth it and exposing those implementation details.
Would supporting this interface create the impression that it covers all known response-related exceptions?
I was thinking of this as something along the lines of SQLException#getErrorCode though in this case they are part of HTTP standards in RFC 9110 and not vendor specific.

carterkozak · 2023-09-14T02:13:57Z

- args.add(SafeArg.of("status", getStatus()));
- args.add(UnsafeArg.of("body", getBody()));
- return Collections.unmodifiableList(args);
+ return List.of(SafeArg.of("status", getStatus()), UnsafeArg.of("body", getBody()));


Any chance you could pull the String.format + collection creation improvements into a separate PR? Would love to get those improvements out, and merging doesn't require me to think as hard about api tradeoffs :-)

no problem, split that out into #1027

schlosna · 2023-09-11T16:33:56Z

Before this PR

Consumers that handle UnknownRemoteException and RemoteException via getStatus() often require duplicative code paths because there is not a shared interface to retrieve the HTTP response codes.

After this PR

==COMMIT_MSG==
(Unknown)RemoteException implement ResponseStatus
==COMMIT_MSG==

Possible downsides?

We already expose getStatus() as public API on both exception types, but do we want to expose this interface as super type?

changelog-app · 2023-09-11T16:33:59Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

(Unknown)RemoteException implement ResponseStatus

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2023-09-14T02:11:47Z

+
+public interface ResponseStatus {
+ /** The HTTP status code of the HTTP response conveying the error. */
+ int getStatus();


I don't have a strong opinion on this. Below is a light braindump thinking through it
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
ServiceException is used within a server to produce a specific error, while RemoteException and UnknownRemoteException are thrown by clients to describe an error response. QosException sort of does both.
Would supporting this interface create the impression that it covers all known response-related exceptions?

I'm on the fence here -- I think there are some cases where it is useful to expose the HTTP status code that triggered the RemoteException, though I think for this case "When in doubt, leave it out". I'm leaning toward just closing this PR.
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
The few places I was looking at where this would be useful would be something along the lines of below. I've refactored those places to just have 2 separate catch clauses that call the same method with the status code.
} catch (RemoteException | UnknownRemoteException e) { // logic using e.getStatus()
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
I could see the status for QosException potentially being useful for some diagnostics, but probably not worth it and exposing those implementation details.
Would supporting this interface create the impression that it covers all known response-related exceptions?
I was thinking of this as something along the lines of SQLException#getErrorCode though in this case they are part of HTTP standards in RFC 9110 and not vendor specific.

carterkozak · 2023-09-14T02:13:57Z

- args.add(SafeArg.of("status", getStatus()));
- args.add(UnsafeArg.of("body", getBody()));
- return Collections.unmodifiableList(args);
+ return List.of(SafeArg.of("status", getStatus()), UnsafeArg.of("body", getBody()));


Any chance you could pull the String.format + collection creation improvements into a separate PR? Would love to get those improvements out, and merging doesn't require me to think as hard about api tradeoffs :-)

no problem, split that out into #1027

schlosna · 2023-09-11T16:33:56Z

Before this PR

Consumers that handle UnknownRemoteException and RemoteException via getStatus() often require duplicative code paths because there is not a shared interface to retrieve the HTTP response codes.

After this PR

==COMMIT_MSG==
(Unknown)RemoteException implement ResponseStatus
==COMMIT_MSG==

Possible downsides?

We already expose getStatus() as public API on both exception types, but do we want to expose this interface as super type?

changelog-app · 2023-09-11T16:33:59Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

(Unknown)RemoteException implement ResponseStatus

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2023-09-14T02:11:47Z

+
+public interface ResponseStatus {
+ /** The HTTP status code of the HTTP response conveying the error. */
+ int getStatus();


I don't have a strong opinion on this. Below is a light braindump thinking through it
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
ServiceException is used within a server to produce a specific error, while RemoteException and UnknownRemoteException are thrown by clients to describe an error response. QosException sort of does both.
Would supporting this interface create the impression that it covers all known response-related exceptions?

I'm on the fence here -- I think there are some cases where it is useful to expose the HTTP status code that triggered the RemoteException, though I think for this case "When in doubt, leave it out". I'm leaning toward just closing this PR.
I'm not sure how much this buys us since most cases will still require us to check/cast to use the method.
The few places I was looking at where this would be useful would be something along the lines of below. I've refactored those places to just have 2 separate catch clauses that call the same method with the status code.
} catch (RemoteException | UnknownRemoteException e) { // logic using e.getStatus()
It's not entirely clear if other exceptions should support this sort of thing (QosException subtypes?).
I could see the status for QosException potentially being useful for some diagnostics, but probably not worth it and exposing those implementation details.
Would supporting this interface create the impression that it covers all known response-related exceptions?
I was thinking of this as something along the lines of SQLException#getErrorCode though in this case they are part of HTTP standards in RFC 9110 and not vendor specific.

carterkozak · 2023-09-14T02:13:57Z

- args.add(SafeArg.of("status", getStatus()));
- args.add(UnsafeArg.of("body", getBody()));
- return Collections.unmodifiableList(args);
+ return List.of(SafeArg.of("status", getStatus()), UnsafeArg.of("body", getBody()));


Any chance you could pull the String.format + collection creation improvements into a separate PR? Would love to get those improvements out, and merging doesn't require me to think as hard about api tradeoffs :-)

no problem, split that out into #1027

schlosna requested a review from carterkozak September 11, 2023 16:33

schlosna added merge when ready autorelease labels Sep 11, 2023

carterkozak reviewed Sep 14, 2023
View reviewed changes

schlosnaand others added 2 commits September 13, 2023 22:27

(Unknown)RemoteException implement ResponseStatus
3feacfc

Add generated changelog entries
5c8ba3e

schlosna force-pushed the ds/response-status branch from a20dbc7 to 5c8ba3eCompare September 14, 2023 02:27

schlosna closed this Sep 14, 2023

schlosna requested a review from carterkozak September 11, 2023 16:33

schlosna added merge when ready autorelease labels Sep 11, 2023

carterkozak reviewed Sep 14, 2023
View reviewed changes

schlosnaand others added 2 commits September 13, 2023 22:27

(Unknown)RemoteException implement ResponseStatus
3feacfc

Add generated changelog entries
5c8ba3e

schlosna force-pushed the ds/response-status branch from a20dbc7 to 5c8ba3eCompare September 14, 2023 02:27

schlosna closed this Sep 14, 2023

schlosna requested a review from carterkozak September 11, 2023 16:33

schlosna added merge when ready autorelease labels Sep 11, 2023

carterkozak reviewed Sep 14, 2023
View reviewed changes

schlosnaand others added 2 commits September 13, 2023 22:27

(Unknown)RemoteException implement ResponseStatus
3feacfc

Add generated changelog entries
5c8ba3e

schlosna force-pushed the ds/response-status branch from a20dbc7 to 5c8ba3eCompare September 14, 2023 02:27

schlosna closed this Sep 14, 2023

schlosna requested a review from carterkozak September 11, 2023 16:33

schlosna added merge when ready autorelease labels Sep 11, 2023

carterkozak reviewed Sep 14, 2023
View reviewed changes

schlosnaand others added 2 commits September 13, 2023 22:27

(Unknown)RemoteException implement ResponseStatus
3feacfc

Add generated changelog entries
5c8ba3e

schlosna force-pushed the ds/response-status branch from a20dbc7 to 5c8ba3eCompare September 14, 2023 02:27

schlosna closed this Sep 14, 2023

schlosna requested a review from carterkozak September 11, 2023 16:33

schlosna added merge when ready autorelease labels Sep 11, 2023

carterkozak reviewed Sep 14, 2023
View reviewed changes

schlosnaand others added 2 commits September 13, 2023 22:27

(Unknown)RemoteException implement ResponseStatus
3feacfc

Add generated changelog entries
5c8ba3e

schlosna force-pushed the ds/response-status branch from a20dbc7 to 5c8ba3eCompare September 14, 2023 02:27

schlosna closed this Sep 14, 2023

schlosna requested a review from carterkozak September 11, 2023 16:33

schlosna added merge when ready autorelease labels Sep 11, 2023

carterkozak reviewed Sep 14, 2023
View reviewed changes

schlosnaand others added 2 commits September 13, 2023 22:27

(Unknown)RemoteException implement ResponseStatus
3feacfc

Add generated changelog entries
5c8ba3e

schlosna force-pushed the ds/response-status branch from a20dbc7 to 5c8ba3eCompare September 14, 2023 02:27

schlosna closed this Sep 14, 2023

schlosna requested a review from carterkozak September 11, 2023 16:33

schlosna added merge when ready autorelease labels Sep 11, 2023

carterkozak reviewed Sep 14, 2023
View reviewed changes

schlosnaand others added 2 commits September 13, 2023 22:27

(Unknown)RemoteException implement ResponseStatus
3feacfc

Add generated changelog entries
5c8ba3e

schlosna force-pushed the ds/response-status branch from a20dbc7 to 5c8ba3eCompare September 14, 2023 02:27

schlosna closed this Sep 14, 2023

schlosna requested a review from carterkozak September 11, 2023 16:33

schlosna added merge when ready autorelease labels Sep 11, 2023

carterkozak reviewed Sep 14, 2023
View reviewed changes

schlosnaand others added 2 commits September 13, 2023 22:27

(Unknown)RemoteException implement ResponseStatus
3feacfc

Add generated changelog entries
5c8ba3e

schlosna force-pushed the ds/response-status branch from a20dbc7 to 5c8ba3eCompare September 14, 2023 02:27

schlosna closed this Sep 14, 2023

Uh oh!

Conversation

schlosna commented Sep 11, 2023

Before this PR

After this PR

Possible downsides?

Uh oh!

changelog-appBot commented Sep 11, 2023• edited by schlosna Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Generate changelog in changelog/@unreleased

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

schlosna commented Sep 11, 2023

Before this PR

After this PR

Possible downsides?

Uh oh!

changelog-appBot commented Sep 11, 2023• edited by schlosna Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Generate changelog in changelog/@unreleased

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

schlosna commented Sep 11, 2023

Before this PR

After this PR

Possible downsides?

Uh oh!

changelog-appBot commented Sep 11, 2023• edited by schlosna Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Generate changelog in changelog/@unreleased

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

changelog-appBot commented Sep 11, 2023•
edited by schlosna
Loading

Generate changelog in `changelog/@unreleased`

changelog-appBot commented Sep 11, 2023•
edited by schlosna
Loading

Generate changelog in `changelog/@unreleased`

changelog-appBot commented Sep 11, 2023•
edited by schlosna
Loading

Generate changelog in `changelog/@unreleased`

changelog-appBot commented Sep 11, 2023•
edited by schlosna
Loading

Generate changelog in `changelog/@unreleased`