WIP cleanup reason deprecation (#2857)

arcjet · Jan 15, 2025 · 9fc8d23 · 9fc8d23
1 parent af75332
commit 9fc8d23
Show file tree

Hide file tree

Showing 7 changed files with 237 additions and 79 deletions.
diff --git a/decorate/index.ts b/decorate/index.ts
@@ -122,9 +122,19 @@ function extractReason(result: ArcjetRuleResult): ArcjetReason {
 }
 
 function isRateLimitReason(
-  reason?: ArcjetReason,
+  reason: ArcjetReason,
 ): reason is ArcjetRateLimitReason {
-  return typeof reason !== "undefined" && reason.isRateLimit();
+  return reason.isRateLimit();
+}
+
+function getRateLimitReason(
+  decision: ArcjetDecision,
+): ArcjetRateLimitReason | undefined {
+  if (decision.isDenied() || decision.isChallenged()) {
+    if (decision.reason.isRateLimit()) {
+      return decision.reason;
+    }
+  }
 }
 
 function nearestLimit(
@@ -211,21 +221,22 @@ export function setRateLimitHeaders(
   } else {
     // For cached decisions, we may not have rule results, but we'd still have
     // the top-level reason.
-    if (isRateLimitReason(decision.reason)) {
+    const rateLimitReason = getRateLimitReason(decision);
+    if (rateLimitReason) {
       if (
-        typeof decision.reason.max !== "number" ||
-        typeof decision.reason.window !== "number" ||
-        typeof decision.reason.remaining !== "number" ||
-        typeof decision.reason.reset !== "number"
+        typeof rateLimitReason.max !== "number" ||
+        typeof rateLimitReason.window !== "number" ||
+        typeof rateLimitReason.remaining !== "number" ||
+        typeof rateLimitReason.reset !== "number"
       ) {
         console.error(
-          format("Invalid rate limit encountered: %o", decision.reason),
+          format("Invalid rate limit encountered: %o", rateLimitReason),
         );
         return;
       }
 
-      limit = toLimitString(decision.reason);
-      policy = toPolicyString([decision.reason.max, decision.reason.window]);
+      limit = toLimitString(rateLimitReason);
+      policy = toPolicyString([rateLimitReason.max, rateLimitReason.window]);
     } else {
       return;
     }

diff --git a/decorate/test/decorate.test.ts b/decorate/test/decorate.test.ts
@@ -3,6 +3,7 @@ import { expect } from "expect";
 import { setRateLimitHeaders } from "../index";
 import {
   ArcjetAllowDecision,
+  ArcjetDenyDecision,
   ArcjetRateLimitReason,
   ArcjetReason,
   ArcjetRuleResult,
@@ -307,7 +308,7 @@ describe("setRateLimitHeaders", () => {
       const headers = new Headers();
       setRateLimitHeaders(
         headers,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({
@@ -332,7 +333,7 @@ describe("setRateLimitHeaders", () => {
       const headers = new Headers();
       setRateLimitHeaders(
         headers,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({
@@ -356,7 +357,7 @@ describe("setRateLimitHeaders", () => {
       const headers = new Headers();
       setRateLimitHeaders(
         headers,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({
@@ -380,7 +381,7 @@ describe("setRateLimitHeaders", () => {
       const headers = new Headers();
       setRateLimitHeaders(
         headers,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({
@@ -404,7 +405,7 @@ describe("setRateLimitHeaders", () => {
       const headers = new Headers();
       setRateLimitHeaders(
         headers,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({
@@ -1023,7 +1024,7 @@ describe("setRateLimitHeaders", () => {
       const resp = new Response();
       setRateLimitHeaders(
         resp,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({
@@ -1048,7 +1049,7 @@ describe("setRateLimitHeaders", () => {
       const resp = new Response();
       setRateLimitHeaders(
         resp,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({
@@ -1072,7 +1073,7 @@ describe("setRateLimitHeaders", () => {
       const resp = new Response();
       setRateLimitHeaders(
         resp,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({
@@ -1096,7 +1097,7 @@ describe("setRateLimitHeaders", () => {
       const resp = new Response();
       setRateLimitHeaders(
         resp,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({
@@ -1120,7 +1121,7 @@ describe("setRateLimitHeaders", () => {
       const resp = new Response();
       setRateLimitHeaders(
         resp,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({
@@ -1834,7 +1835,7 @@ describe("setRateLimitHeaders", () => {
       const resp = new OutgoingMessage();
       setRateLimitHeaders(
         resp,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({
@@ -1859,7 +1860,7 @@ describe("setRateLimitHeaders", () => {
       const resp = new OutgoingMessage();
       setRateLimitHeaders(
         resp,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({
@@ -1883,7 +1884,7 @@ describe("setRateLimitHeaders", () => {
       const resp = new OutgoingMessage();
       setRateLimitHeaders(
         resp,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({
@@ -1907,7 +1908,7 @@ describe("setRateLimitHeaders", () => {
       const resp = new OutgoingMessage();
       setRateLimitHeaders(
         resp,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({
@@ -1931,7 +1932,7 @@ describe("setRateLimitHeaders", () => {
       const resp = new OutgoingMessage();
       setRateLimitHeaders(
         resp,
-        new ArcjetAllowDecision({
+        new ArcjetDenyDecision({
           results: [],
           ttl: 0,
           reason: new ArcjetRateLimitReason({

diff --git a/protocol/client.ts b/protocol/client.ts
@@ -115,7 +115,6 @@ export function createClient(options: ClientOptions): Client {
           runtime: context.runtime,
           ttl: decision.ttl,
           conclusion: decision.conclusion,
-          reason: decision.reason,
           ruleResults: decision.results,
         },
         "Decide response",

diff --git a/protocol/convert.ts b/protocol/convert.ts
@@ -392,13 +392,45 @@ export function ArcjetRuleResultFromProtocol(
 }
 
 export function ArcjetDecisionToProtocol(decision: ArcjetDecision): Decision {
-  return new Decision({
-    id: decision.id,
-    ttl: decision.ttl,
-    conclusion: ArcjetConclusionToProtocol(decision.conclusion),
-    reason: decision.reason && ArcjetReasonToProtocol(decision.reason),
-    ruleResults: decision.results.map(ArcjetRuleResultToProtocol),
-  });
+  if (decision.isDenied()) {
+    return new Decision({
+      id: decision.id,
+      ttl: decision.ttl,
+      conclusion: ArcjetConclusionToProtocol(decision.conclusion),
+      reason: ArcjetReasonToProtocol(decision.reason),
+      ruleResults: decision.results.map(ArcjetRuleResultToProtocol),
+    });
+  }
+
+  if (decision.isChallenged()) {
+    return new Decision({
+      id: decision.id,
+      ttl: decision.ttl,
+      conclusion: ArcjetConclusionToProtocol(decision.conclusion),
+      reason: ArcjetReasonToProtocol(decision.reason),
+      ruleResults: decision.results.map(ArcjetRuleResultToProtocol),
+    });
+  }
+
+  if (decision.isErrored()) {
+    return new Decision({
+      id: decision.id,
+      ttl: decision.ttl,
+      conclusion: ArcjetConclusionToProtocol(decision.conclusion),
+      ruleResults: decision.results.map(ArcjetRuleResultToProtocol),
+    });
+  }
+
+  if (decision.isAllowed()) {
+    return new Decision({
+      id: decision.id,
+      ttl: decision.ttl,
+      conclusion: ArcjetConclusionToProtocol(decision.conclusion),
+      ruleResults: decision.results.map(ArcjetRuleResultToProtocol),
+    });
+  }
+
+  return new Decision();
 }
 
 export function ArcjetIpDetailsFromProtocol(
@@ -667,6 +699,7 @@ export function ArcjetRuleToProtocol<Props extends { [key: string]: unknown }>(
       rule: {
         case: "sensitiveInfo",
         value: {
+          mode: ArcjetModeToProtocol(rule.mode),
           allow: rule.allow,
           deny: rule.deny,
         },

diff --git a/protocol/index.ts b/protocol/index.ts
@@ -575,10 +575,6 @@ export class ArcjetIpDetails {
  * the decision in the Arcjet dashboard.
  * @property `conclusion` - Arcjet's conclusion about the request. This will be
  * one of `"ALLOW"`, `"DENY"`, `"CHALLENGE"`, or `"ERROR"`.
- * @property `reason` - A structured data type about the reason for the
- * decision. One of: {@link ArcjetRateLimitReason}, {@link ArcjetEdgeRuleReason},
- * {@link ArcjetBotReason}, {@link ArcjetShieldReason},
- * {@link ArcjetEmailReason}, or {@link ArcjetErrorReason}.
  * @property `ttl` - The duration in milliseconds this decision should be
  * considered valid, also known as time-to-live.
  * @property `results` - Each separate {@link ArcjetRuleResult} can be found here
@@ -599,7 +595,6 @@ export abstract class ArcjetDecision {
   ip: ArcjetIpDetails;
 
   abstract conclusion: ArcjetConclusion;
-  abstract reason: ArcjetReason | undefined;
 
   constructor(init: {
     id?: string;
@@ -637,7 +632,9 @@ export abstract class ArcjetDecision {
 
 export class ArcjetAllowDecision extends ArcjetDecision {
   conclusion = "ALLOW" as const;
-  /** @deprecated: use results instead */
+  /**
+   * @deprecated Iterate rule results via `decision.results` instead.
+   **/
   reason: ArcjetReason | undefined;
 
   constructor(init: {
@@ -651,10 +648,6 @@ export class ArcjetAllowDecision extends ArcjetDecision {
 
     this.reason = init.reason;
   }
-
-  hasReason(): this is ArcjetAllowDecision & { reason: ArcjetReason } {
-    return this.reason !== undefined;
-  }
 }
 
 export class ArcjetDenyDecision extends ArcjetDecision {
@@ -692,14 +685,16 @@ export class ArcjetChallengeDecision extends ArcjetDecision {
 
 export class ArcjetErrorDecision extends ArcjetDecision {
   conclusion = "ERROR" as const;
-  /** @deprecated: use results instead */
+  /**
+   * @deprecated Iterate rule results via `decision.results` instead.
+   * */
   reason: ArcjetErrorReason | undefined;
 
   constructor(init: {
     id?: string;
     results: ArcjetRuleResult[];
     ttl: number;
-    reason: ArcjetErrorReason;
+    reason?: ArcjetErrorReason;
     ip?: ArcjetIpDetails;
   }) {
     super(init);

diff --git a/protocol/test/client.test.ts b/protocol/test/client.test.ts
@@ -565,7 +565,8 @@ describe("createClient", () => {
     const decision = await client.decide(context, details, []);
 
     expect(decision.isErrored()).toBe(true);
-    expect(decision.reason).toMatchObject({
+    // Duplicated `isErrored()` narrowing because the assertion library isn't assertion aware
+    expect(decision.isErrored() && decision.reason).toMatchObject({
       message: "Unknown error occurred",
     });
   });
@@ -619,7 +620,8 @@ describe("createClient", () => {
     const decision = await client.decide(context, details, []);
 
     expect(decision.isErrored()).toBe(true);
-    expect(decision.reason).toMatchObject({
+    // Duplicated `isErrored()` narrowing because the assertion library isn't assertion aware
+    expect(decision.isErrored() && decision.reason).toMatchObject({
       message: "Boom!",
     });
   });
@@ -785,7 +787,6 @@ describe("createClient", () => {
         decision: {
           id: decision.id,
           conclusion: Conclusion.ALLOW,
-          reason: new Reason(),
           ruleResults: [],
         },
       }),
@@ -921,14 +922,6 @@ describe("createClient", () => {
         decision: {
           id: decision.id,
           conclusion: Conclusion.ERROR,
-          reason: new Reason({
-            reason: {
-              case: "error",
-              value: {
-                message: "Failure",
-              },
-            },
-          }),
           ruleResults: [],
         },
       }),
@@ -1057,12 +1050,7 @@ describe("createClient", () => {
           ...details,
           headers: { "user-agent": "curl/8.1.2" },
         },
-        decision: {
-          id: decision.id,
-          conclusion: Conclusion.UNSPECIFIED,
-          reason: new Reason(),
-          ruleResults: [],
-        },
+        decision: {},
       }),
       expect.anything(),
     ]);