Skip to content
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.

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

WIP cleanup reason deprecation #2857

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 21 additions & 10 deletions decorate/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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;
}
Expand Down
31 changes: 16 additions & 15 deletions decorate/test/decorate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { expect } from "expect";
import { setRateLimitHeaders } from "../index";
import {
ArcjetAllowDecision,
ArcjetDenyDecision,
ArcjetRateLimitReason,
ArcjetReason,
ArcjetRuleResult,
Expand Down Expand Up @@ -307,7 +308,7 @@ describe("setRateLimitHeaders", () => {
const headers = new Headers();
setRateLimitHeaders(
headers,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand All @@ -332,7 +333,7 @@ describe("setRateLimitHeaders", () => {
const headers = new Headers();
setRateLimitHeaders(
headers,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand All @@ -356,7 +357,7 @@ describe("setRateLimitHeaders", () => {
const headers = new Headers();
setRateLimitHeaders(
headers,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand All @@ -380,7 +381,7 @@ describe("setRateLimitHeaders", () => {
const headers = new Headers();
setRateLimitHeaders(
headers,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand All @@ -404,7 +405,7 @@ describe("setRateLimitHeaders", () => {
const headers = new Headers();
setRateLimitHeaders(
headers,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand Down Expand Up @@ -1023,7 +1024,7 @@ describe("setRateLimitHeaders", () => {
const resp = new Response();
setRateLimitHeaders(
resp,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand All @@ -1048,7 +1049,7 @@ describe("setRateLimitHeaders", () => {
const resp = new Response();
setRateLimitHeaders(
resp,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand All @@ -1072,7 +1073,7 @@ describe("setRateLimitHeaders", () => {
const resp = new Response();
setRateLimitHeaders(
resp,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand All @@ -1096,7 +1097,7 @@ describe("setRateLimitHeaders", () => {
const resp = new Response();
setRateLimitHeaders(
resp,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand All @@ -1120,7 +1121,7 @@ describe("setRateLimitHeaders", () => {
const resp = new Response();
setRateLimitHeaders(
resp,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand Down Expand Up @@ -1834,7 +1835,7 @@ describe("setRateLimitHeaders", () => {
const resp = new OutgoingMessage();
setRateLimitHeaders(
resp,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand All @@ -1859,7 +1860,7 @@ describe("setRateLimitHeaders", () => {
const resp = new OutgoingMessage();
setRateLimitHeaders(
resp,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand All @@ -1883,7 +1884,7 @@ describe("setRateLimitHeaders", () => {
const resp = new OutgoingMessage();
setRateLimitHeaders(
resp,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand All @@ -1907,7 +1908,7 @@ describe("setRateLimitHeaders", () => {
const resp = new OutgoingMessage();
setRateLimitHeaders(
resp,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand All @@ -1931,7 +1932,7 @@ describe("setRateLimitHeaders", () => {
const resp = new OutgoingMessage();
setRateLimitHeaders(
resp,
new ArcjetAllowDecision({
new ArcjetDenyDecision({
results: [],
ttl: 0,
reason: new ArcjetRateLimitReason({
Expand Down
1 change: 0 additions & 1 deletion protocol/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
47 changes: 40 additions & 7 deletions protocol/convert.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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,
},
Expand Down
19 changes: 7 additions & 12 deletions protocol/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -599,7 +595,6 @@ export abstract class ArcjetDecision {
ip: ArcjetIpDetails;

abstract conclusion: ArcjetConclusion;
abstract reason: ArcjetReason | undefined;

constructor(init: {
id?: string;
Expand Down Expand Up @@ -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: {
Expand All @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down
22 changes: 5 additions & 17 deletions protocol/test/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
});
});
Expand Down Expand Up @@ -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!",
});
});
Expand Down Expand Up @@ -785,7 +787,6 @@ describe("createClient", () => {
decision: {
id: decision.id,
conclusion: Conclusion.ALLOW,
reason: new Reason(),
ruleResults: [],
},
}),
Expand Down Expand Up @@ -921,14 +922,6 @@ describe("createClient", () => {
decision: {
id: decision.id,
conclusion: Conclusion.ERROR,
reason: new Reason({
reason: {
case: "error",
value: {
message: "Failure",
},
},
}),
ruleResults: [],
},
}),
Expand Down Expand Up @@ -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(),
]);
Expand Down
Loading
Loading