Skip to content

Commit 6f32900

Browse files
umair-ablyumair-ably
committed
Address CodeRabbit review feedback for push commands
- Guard deprecation warning in set-apns-p12 for JSON mode - Use formatWarning() and formatClientId() in push channels list - Validate --data flag parses to a JSON object in push devices save - Validate all JSON flags (--recipient, --payload, --data, --apns, --fcm, --web) parse to objects in push publish - Let E2E test setup failures propagate instead of silently skipping
1 parent 635c4e3 commit 6f32900

6 files changed

Lines changed: 80 additions & 35 deletions

File tree

‎src/commands/apps/set-apns-p12.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {
88
formatProgress,
99
formatResource,
1010
formatSuccess,
11+
formatWarning,
1112
} from "../../utils/output.js";
1213

1314
export default class AppsSetApnsP12Command extends ControlBaseCommand {
@@ -49,9 +50,13 @@ export default class AppsSetApnsP12Command extends ControlBaseCommand {
4950
async run(): Promise<void> {
5051
const { args, flags } = await this.parse(AppsSetApnsP12Command);
5152

52-
this.warn(
53-
'This command is deprecated. Use "ably push config set-apns" instead.',
54-
);
53+
if (!this.shouldOutputJson(flags)) {
54+
this.logToStderr(
55+
formatWarning(
56+
'This command is deprecated. Use "ably push config set-apns" instead.',
57+
),
58+
);
59+
}
5560

5661
// Display authentication information
5762
this.showAuthInfoIfNeeded(flags);

‎src/commands/push/channels/list.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,15 @@ import { AblyBaseCommand } from "../../../base-command.js";
44
import { productApiFlags } from "../../../flags.js";
55
import { BaseFlags } from "../../../types/cli.js";
66
import {
7+
formatClientId,
78
formatCountLabel,
89
formatHeading,
910
formatLabel,
1011
formatLimitWarning,
1112
formatProgress,
1213
formatResource,
1314
formatSuccess,
15+
formatWarning,
1416
} from "../../../utils/output.js";
1517

1618
export default class PushChannelsList extends AblyBaseCommand {
@@ -71,7 +73,7 @@ export default class PushChannelsList extends AblyBaseCommand {
7173
}
7274

7375
if (subscriptions.length === 0) {
74-
this.log("No subscriptions found.");
76+
this.log(formatWarning("No subscriptions found."));
7577
return;
7678
}
7779

@@ -90,7 +92,9 @@ export default class PushChannelsList extends AblyBaseCommand {
9092
if (sub.deviceId)
9193
this.log(` ${formatLabel("Device ID")} ${sub.deviceId}`);
9294
if (sub.clientId)
93-
this.log(` ${formatLabel("Client ID")} ${sub.clientId}`);
95+
this.log(
96+
` ${formatLabel("Client ID")} ${formatClientId(sub.clientId)}`,
97+
);
9498
this.log("");
9599
}
96100

‎src/commands/push/devices/save.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,14 +225,25 @@ export default class PushDevicesSave extends AblyBaseCommand {
225225
jsonString = fs.readFileSync(filePath, "utf8");
226226
}
227227

228+
let parsed: unknown;
228229
try {
229-
return JSON.parse(jsonString) as Record<string, unknown>;
230+
parsed = JSON.parse(jsonString);
230231
} catch {
231232
this.fail(
232233
"--data must be valid JSON or a path to a JSON file (prefixed with @)",
233234
flags as BaseFlags,
234235
"pushDeviceSave",
235236
);
236237
}
238+
239+
if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
240+
this.fail(
241+
"--data must be a JSON object",
242+
flags as BaseFlags,
243+
"pushDeviceSave",
244+
);
245+
}
246+
247+
return parsed as Record<string, unknown>;
237248
}
238249
}

‎src/commands/push/publish.ts‎

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -93,10 +93,14 @@ export default class PushPublish extends AblyBaseCommand {
9393
recipient = { clientId: flags["client-id"] };
9494
} else {
9595
try {
96-
recipient = JSON.parse(flags.recipient!) as Record<string, unknown>;
96+
const parsed = JSON.parse(flags.recipient!);
97+
if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
98+
throw new Error("not an object");
99+
}
100+
recipient = parsed as Record<string, unknown>;
97101
} catch {
98102
this.fail(
99-
"--recipient must be valid JSON",
103+
"--recipient must be a valid JSON object",
100104
flags as BaseFlags,
101105
"pushPublish",
102106
);
@@ -107,10 +111,14 @@ export default class PushPublish extends AblyBaseCommand {
107111
let payload: Record<string, unknown>;
108112
if (flags.payload) {
109113
try {
110-
payload = JSON.parse(flags.payload) as Record<string, unknown>;
114+
const parsed = JSON.parse(flags.payload);
115+
if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
116+
throw new Error("not an object");
117+
}
118+
payload = parsed as Record<string, unknown>;
111119
} catch {
112120
this.fail(
113-
"--payload must be valid JSON",
121+
"--payload must be a valid JSON object",
114122
flags as BaseFlags,
115123
"pushPublish",
116124
);
@@ -133,10 +141,18 @@ export default class PushPublish extends AblyBaseCommand {
133141

134142
if (flags.data) {
135143
try {
136-
payload.data = JSON.parse(flags.data);
144+
const parsed = JSON.parse(flags.data);
145+
if (
146+
!parsed ||
147+
typeof parsed !== "object" ||
148+
Array.isArray(parsed)
149+
) {
150+
throw new Error("not an object");
151+
}
152+
payload.data = parsed;
137153
} catch {
138154
this.fail(
139-
"--data must be valid JSON",
155+
"--data must be a valid JSON object",
140156
flags as BaseFlags,
141157
"pushPublish",
142158
);
@@ -147,32 +163,44 @@ export default class PushPublish extends AblyBaseCommand {
147163
// Add platform-specific overrides
148164
if (flags.apns) {
149165
try {
150-
payload.apns = JSON.parse(flags.apns);
166+
const parsed = JSON.parse(flags.apns);
167+
if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
168+
throw new Error("not an object");
169+
}
170+
payload.apns = parsed;
151171
} catch {
152172
this.fail(
153-
"--apns must be valid JSON",
173+
"--apns must be a valid JSON object",
154174
flags as BaseFlags,
155175
"pushPublish",
156176
);
157177
}
158178
}
159179
if (flags.fcm) {
160180
try {
161-
payload.fcm = JSON.parse(flags.fcm);
181+
const parsed = JSON.parse(flags.fcm);
182+
if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
183+
throw new Error("not an object");
184+
}
185+
payload.fcm = parsed;
162186
} catch {
163187
this.fail(
164-
"--fcm must be valid JSON",
188+
"--fcm must be a valid JSON object",
165189
flags as BaseFlags,
166190
"pushPublish",
167191
);
168192
}
169193
}
170194
if (flags.web) {
171195
try {
172-
payload.web = JSON.parse(flags.web);
196+
const parsed = JSON.parse(flags.web);
197+
if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
198+
throw new Error("not an object");
199+
}
200+
payload.web = parsed;
173201
} catch {
174202
this.fail(
175-
"--web must be valid JSON",
203+
"--web must be a valid JSON object",
176204
flags as BaseFlags,
177205
"pushPublish",
178206
);

‎test/e2e/connections/connections.test.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,8 @@ describe("Connections E2E Tests", () => {
170170
delete monitorEnv.ABLY_CLI_TEST_MODE;
171171
const apiKey = process.env.E2E_ABLY_API_KEY;
172172
if (!apiKey) {
173-
throw new Error("E2E_ABLY_API_KEY environment variable is required");
173+
console.log("E2E_ABLY_API_KEY environment variable is required - skipping");
174+
return;
174175
}
175176

176177
// Use connection-lifecycle command which uses the correct meta channel

‎test/e2e/push/push-config-e2e.test.ts‎

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -41,23 +41,19 @@ describe("Push Config E2E Tests", () => {
4141
logErrors: false,
4242
});
4343

44-
try {
45-
// Create a dedicated test app for push config tests
46-
const appName = `E2E Push Config Test ${Date.now()}`;
47-
const createResult = await runCommand(
48-
["apps", "create", "--name", appName, "--json"],
49-
{
50-
env: { ABLY_ACCESS_TOKEN: accessToken },
51-
},
52-
);
44+
// Create a dedicated test app for push config tests
45+
// Let setup failures propagate — only missing credentials should skip
46+
const appName = `E2E Push Config Test ${Date.now()}`;
47+
const createResult = await runCommand(
48+
["apps", "create", "--name", appName, "--json"],
49+
{
50+
env: { ABLY_ACCESS_TOKEN: accessToken },
51+
},
52+
);
5353

54-
const result = JSON.parse(createResult.stdout);
55-
testAppId = result.app.id;
56-
console.log(`Created test app for push config: ${testAppId}`);
57-
} catch (error) {
58-
console.error("Failed to create test app:", error);
59-
shouldSkip = true;
60-
}
54+
const result = JSON.parse(createResult.stdout);
55+
testAppId = result.app.id;
56+
console.log(`Created test app for push config: ${testAppId}`);
6157
});
6258

6359
afterAll(async () => {

0 commit comments

Comments
 (0)