encode/decode DeadlineExpiredException types #30

bjlaub · 2025-02-12T19:35:55Z

No description provided.

changelog-app · 2025-02-12T19:36:00Z

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

add header for hinting at expiration reasons

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2025-02-12T19:54:51Z

deadlines/src/main/java/com/palantir/deadlines/DeadlineExpiredReasons.java

+    public interface ResponseEncodingAdapter<RESPONSE> {
+        void setHeader(RESPONSE response, String headerName, String headerValue);
+    }


We could control the status code here as well

bjlaub · 2025-02-12T20:31:39Z

deadlines/src/main/java/com/palantir/deadlines/DeadlineExpiredReasons.java

+    public interface ResponseEncodingAdapter<RESPONSE> {
+        void setHeader(RESPONSE response, String headerName, String headerValue);
+
+        void setStatus(RESPONSE response, int status);


perhaps we should invert this and use the visitor style a la QosException

to expand on that: reason being that in e.g. palantir/conjure-java#2454, we're leveraging the private ConjureExceptions#writeResponse which accepts an http status as a parameter, but also contains useful logic that we probably want to re-use involved in writing the response body.

instead of including a setStatus() method on the encoder, we could expose a basic method here:

public int httpStatusFor(DeadlineExpiredException exception) { if (exception instanceof DeadlineExpiredException.External) { return 400; } else if (exception instanceof DeadlineExpiredException.Internal) { return 500; } return 500; }

thoughts?

I think either way could work. I don't think we necessarily need to send a response at all (similarly to QosException), although I suppose an errorInstanceId might be nice to quickly find the failures propagation path back to clients.

updated to use the visitor thing. i don't love it... but i guess it works

i think this is simpler: 8fbd209

carterkozak · 2025-02-13T15:07:12Z

deadlines/src/main/java/com/palantir/deadlines/DeadlineExpiredException.java

@@ -31,6 +31,8 @@ private DeadlineExpiredException(String message) {
        super(message);
    }

+    public abstract int httpStatusCode();


Slight preference to put these into a helper method on Deadlines.java along the lines of int getHttpStatusCode(DeadlineExpiredException), that way the exceptions themselves aren't directly coupled to http.

works for me, will fix

…abstract method

carterkozak · 2025-02-13T18:36:13Z

deadlines/src/main/java/com/palantir/deadlines/DeadlineExpiredReasons.java

+    public static int getHttpStatusCode(DeadlineExpiredException.External _exception) {
+        // external deadline expiration is considered a client error
+        return 400;
+    }
+
+    public static int getHttpStatusCode(DeadlineExpiredException.Internal _exception) {
+        // internal deadline expiration is considered a server error
+        return 500;
+    }


Thoughts on merging this to public static int getHttpStatusCode(DeadlineExpiredException exception) { so we don't need to inspect the type to get the correct status code? Makes future additions a bit easier imo

sure, fixed: 7669548

carterkozak · 2025-02-14T15:00:22Z

deadlines/src/main/java/com/palantir/deadlines/DeadlineExpiredReasons.java

+        return reason.flatMap(s -> switch (s) {
+            case "external" -> Optional.of(DeadlineExpiredException.external());
+            case "internal" -> Optional.of(DeadlineExpiredException.internal());
+            default -> Optional.empty();
+        });


Should we check the status code for internal/external as well? Ideally we're as precise as possible, only producing DeadlineExpiredException.external when the response status is 400, and internal when the status is 500

carterkozak · 2025-02-14T15:06:01Z

deadlines/src/main/java/com/palantir/deadlines/DeadlineExpiredReasons.java

+    }
+
+    public interface ResponseDecodingAdapter<RESPONSE> {
+        Optional<String> getFirstHeader(RESPONSE response, String headerName);


Thoughts on using @Nullable here instead of optional? I sort of wish we'd done that on the other adapter to avoid unnecessary optional allocation.

We could call it @Nullable String maybeFirstHeaer(RESPONSE response, String headerName), allowing us to add an equivalent method on the existing adapter in a follow-up

carterkozak · 2025-02-14T15:30:59Z

deadlines/src/main/java/com/palantir/deadlines/DeadlineExpiredReasons.java

+        @Deprecated
+        default Optional<String> getFirstHeader(RESPONSE response, String headerName) {
+            return Optional.empty();
+        }


Since this type is new in this PR, we can remove this method

autorelease3 · 2025-02-14T16:40:29Z

Released 0.7.0

add header for hinting at expiration reasons

e848dd7

bjlaub mentioned this pull request Feb 12, 2025

handle DeadlineExpiredException in standard exception mapper palantir/conjure-java#2454

Merged

carterkozak reviewed Feb 12, 2025

View reviewed changes

Brian Laub added 2 commits February 12, 2025 15:24

encode http status code in response

838afc4

fixup

d8de1e1

bjlaub changed the title ~~add header for hinting at expiration reasons~~ encode/decode DeadlineExpiredException types Feb 12, 2025

bjlaub marked this pull request as ready for review February 12, 2025 20:28

Add generated changelog entries

1194040

bjlaub commented Feb 12, 2025

View reviewed changes

Brian Laub added 2 commits February 12, 2025 19:18

update to use visitor for getting http status code

a9e6cf0

try something different

8fbd209

carterkozak reviewed Feb 13, 2025

View reviewed changes

use static getter for exception types to map to http code instead of …

c9c4ad6

…abstract method

carterkozak reviewed Feb 13, 2025

View reviewed changes

Brian Laub added 2 commits February 13, 2025 14:03

pr feedback

7669548

fix

5ea8ca5

bjlaub added autorelease merge when ready and removed merge when ready labels Feb 14, 2025

carterkozak reviewed Feb 14, 2025

View reviewed changes

refactor

f82563c

carterkozak reviewed Feb 14, 2025

View reviewed changes

remove unused methods

5ebd95d

carterkozak approved these changes Feb 14, 2025

View reviewed changes

bjlaub added the merge when ready label Feb 14, 2025

bulldozer-bot bot merged commit e269f56 into develop Feb 14, 2025
5 checks passed

bulldozer-bot bot deleted the blaub/exception-types-header branch February 14, 2025 16:40

encode/decode DeadlineExpiredException types #30

encode/decode DeadlineExpiredException types #30

Uh oh!

Conversation

bjlaub commented Feb 12, 2025

Uh oh!

changelog-app bot commented Feb 12, 2025 • edited by bjlaub 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!

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!

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!

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!

Uh oh!

autorelease3 bot commented Feb 14, 2025

Uh oh!

Uh oh!

changelog-app bot commented Feb 12, 2025 •

edited by bjlaub

Loading

Generate changelog in `changelog/@unreleased`