Skip to content

Commit 57b49de

Browse files
ZetazzzZetazzz
committed
refactor: preserve SMS config precedence
1 parent c8c5d93 commit 57b49de

3 files changed

Lines changed: 119 additions & 59 deletions

File tree

‎graphql/env/__tests__/merge.test.ts‎

Lines changed: 70 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,10 @@ import * as os from 'os';
55
import * as path from 'path';
66

77
const writeConfig = (dir: string, config: Record<string, unknown>): void => {
8-
fs.writeFileSync(path.join(dir, 'pgpm.json'), JSON.stringify(config, null, 2));
8+
fs.writeFileSync(
9+
path.join(dir, 'pgpm.json'),
10+
JSON.stringify(config, null, 2)
11+
);
912
};
1013

1114
describe('getEnvOptions', () => {
@@ -23,31 +26,31 @@ describe('getEnvOptions', () => {
2326
writeConfig(tempDir, {
2427
pg: {
2528
host: 'config-host',
26-
database: 'config-db'
29+
database: 'config-db',
2730
},
2831
server: {
29-
port: 4000
32+
port: 4000,
3033
},
3134
graphile: {
32-
schema: ['config_schema']
35+
schema: ['config_schema'],
3336
},
3437
features: {
35-
simpleInflection: false
38+
simpleInflection: false,
3639
},
3740
api: {
3841
enableServicesApi: false,
3942
isPublic: false,
40-
metaSchemas: ['config_meta']
43+
metaSchemas: ['config_meta'],
4144
},
4245
sms: {
4346
provider: 'devsms',
4447
senderId: 'ConfigSender',
4548
requestTimeoutMs: 3000,
4649
dryRun: false,
4750
devsms: {
48-
baseUrl: 'http://config-devsms:4000'
49-
}
50-
}
51+
baseUrl: 'http://config-devsms:4000',
52+
},
53+
},
5154
});
5255

5356
const testEnv: NodeJS.ProcessEnv = {
@@ -67,34 +70,34 @@ describe('getEnvOptions', () => {
6770
SMS_SENDER_ID: 'EnvSender',
6871
SMS_REQUEST_TIMEOUT_MS: '4000',
6972
SEND_SMS_DRY_RUN: 'true',
70-
DEVSMS_BASE_URL: 'http://env-devsms:4000'
73+
DEVSMS_BASE_URL: 'http://env-devsms:4000',
7174
};
7275

7376
const result = getEnvOptions(
7477
{
7578
db: {
76-
cwd: '<CWD>'
79+
cwd: '<CWD>',
7780
},
7881
pg: {
79-
host: 'override-host'
82+
host: 'override-host',
8083
},
8184
server: {
82-
port: 5000
85+
port: 5000,
8386
},
8487
graphile: {
85-
schema: ['override_schema']
88+
schema: ['override_schema'],
8689
},
8790
features: {
88-
oppositeBaseNames: false
91+
oppositeBaseNames: false,
8992
},
9093
api: {
9194
enableServicesApi: false,
92-
defaultDatabaseId: 'override_db'
95+
defaultDatabaseId: 'override_db',
9396
},
9497
sms: {
9598
senderId: 'OverrideSender',
96-
requestTimeoutMs: 9000
97-
}
99+
requestTimeoutMs: 9000,
100+
},
98101
},
99102
tempDir,
100103
testEnv
@@ -107,36 +110,39 @@ describe('getEnvOptions', () => {
107110
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'graphql-env-replace-'));
108111
writeConfig(tempDir, {
109112
graphile: {
110-
schema: ['config_schema', 'shared_schema']
113+
schema: ['config_schema', 'shared_schema'],
111114
},
112115
api: {
113116
exposedSchemas: ['public', 'shared'],
114-
metaSchemas: ['metaschema_public', 'services_public', 'config_meta']
115-
}
117+
metaSchemas: ['metaschema_public', 'services_public', 'config_meta'],
118+
},
116119
});
117120

118121
const testEnv: NodeJS.ProcessEnv = {
119122
GRAPHILE_SCHEMA: 'shared_schema,env_schema',
120123
API_EXPOSED_SCHEMAS: 'shared,env_schema',
121-
API_META_SCHEMAS: 'services_public,env_meta'
124+
API_META_SCHEMAS: 'services_public,env_meta',
122125
};
123126

124127
const result = getEnvOptions(
125128
{
126129
graphile: {
127-
schema: ['override_schema', 'shared_schema']
130+
schema: ['override_schema', 'shared_schema'],
128131
},
129132
api: {
130133
exposedSchemas: ['public', 'override_schema'],
131-
metaSchemas: ['env_meta', 'override_meta']
132-
}
134+
metaSchemas: ['env_meta', 'override_meta'],
135+
},
133136
},
134137
tempDir,
135138
testEnv
136139
);
137140

138141
// Arrays are replaced, not merged - overrides win completely
139-
expect(result.graphile?.schema).toEqual(['override_schema', 'shared_schema']);
142+
expect(result.graphile?.schema).toEqual([
143+
'override_schema',
144+
'shared_schema',
145+
]);
140146
expect(result.api?.exposedSchemas).toEqual(['public', 'override_schema']);
141147
expect(result.api?.metaSchemas).toEqual(['env_meta', 'override_meta']);
142148
});
@@ -147,7 +153,7 @@ describe('getEnvOptions', () => {
147153
SMS_SENDER_ID: 'LocalSender',
148154
SMS_REQUEST_TIMEOUT_MS: '2500',
149155
SEND_SMS_DRY_RUN: 'true',
150-
DEVSMS_BASE_URL: 'http://localhost:4000'
156+
DEVSMS_BASE_URL: 'http://localhost:4000',
151157
});
152158

153159
expect(result.sms).toEqual({
@@ -156,14 +162,14 @@ describe('getEnvOptions', () => {
156162
requestTimeoutMs: 2500,
157163
dryRun: true,
158164
devsms: {
159-
baseUrl: 'http://localhost:4000'
160-
}
165+
baseUrl: 'http://localhost:4000',
166+
},
161167
});
162168
});
163169

164170
it('accepts custom SMS provider names', () => {
165171
const result = getGraphQLEnvVars({
166-
SMS_PROVIDER: 'custom-sms-gateway'
172+
SMS_PROVIDER: 'custom-sms-gateway',
167173
});
168174

169175
expect(result.sms?.provider).toBe('custom-sms-gateway');
@@ -178,22 +184,22 @@ describe('getEnvOptions', () => {
178184
requestTimeoutMs: 3000,
179185
dryRun: false,
180186
devsms: {
181-
baseUrl: 'http://config-devsms:4000'
182-
}
183-
}
187+
baseUrl: 'http://config-devsms:4000',
188+
},
189+
},
184190
});
185191

186192
const result = getEnvOptions(
187193
{
188194
sms: {
189-
requestTimeoutMs: 9000
190-
}
195+
requestTimeoutMs: 9000,
196+
},
191197
},
192198
tempDir,
193199
{
194200
SMS_SENDER_ID: 'EnvSender',
195201
SEND_SMS_DRY_RUN: 'true',
196-
DEVSMS_BASE_URL: 'http://env-devsms:4000'
202+
DEVSMS_BASE_URL: 'http://env-devsms:4000',
197203
}
198204
);
199205

@@ -203,8 +209,8 @@ describe('getEnvOptions', () => {
203209
requestTimeoutMs: 9000,
204210
dryRun: true,
205211
devsms: {
206-
baseUrl: 'http://env-devsms:4000'
207-
}
212+
baseUrl: 'http://env-devsms:4000',
213+
},
208214
});
209215
});
210216

@@ -214,7 +220,7 @@ describe('getEnvOptions', () => {
214220

215221
try {
216222
const result = getEnvOptions({}, process.cwd(), {
217-
SMS_PROVIDER: 'devsms'
223+
SMS_PROVIDER: 'devsms',
218224
});
219225

220226
expect(result.sms?.provider).toBe('devsms');
@@ -232,15 +238,36 @@ describe('getEnvOptions', () => {
232238

233239
expect(result.sms).toEqual({
234240
requestTimeoutMs: 5000,
235-
dryRun: false
241+
dryRun: false,
236242
});
237243
});
238244

239-
it('uses shared number parsing behavior for invalid SMS_REQUEST_TIMEOUT_MS values', () => {
245+
it('omits an invalid SMS timeout from partial env overrides', () => {
240246
const result = getGraphQLEnvVars({
241-
SMS_REQUEST_TIMEOUT_MS: '5s'
247+
SMS_REQUEST_TIMEOUT_MS: '5s',
242248
});
243249

244-
expect(result.sms?.requestTimeoutMs).toBeUndefined();
250+
expect(result.sms).toBeUndefined();
251+
});
252+
253+
it('does not let absent or invalid SMS env values override config', () => {
254+
tempDir = fs.mkdtempSync(
255+
path.join(os.tmpdir(), 'graphql-env-sms-defaults-')
256+
);
257+
writeConfig(tempDir, {
258+
sms: {
259+
requestTimeoutMs: 3000,
260+
dryRun: true,
261+
},
262+
});
263+
264+
const result = getEnvOptions({}, tempDir, {
265+
SMS_REQUEST_TIMEOUT_MS: '5s',
266+
});
267+
268+
expect(result.sms).toEqual({
269+
requestTimeoutMs: 3000,
270+
dryRun: true,
271+
});
245272
});
246273
});

‎graphql/env/src/env.ts‎

Lines changed: 42 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ import { parseEnvBoolean, parseEnvNumber } from '12factor-env';
44
/**
55
* @param env - Environment object to read from (defaults to process.env for backwards compatibility)
66
*/
7-
export const getGraphQLEnvVars = (env: NodeJS.ProcessEnv = process.env): Partial<ConstructiveOptions> => {
7+
export const getGraphQLEnvVars = (
8+
env: NodeJS.ProcessEnv = process.env
9+
): Partial<ConstructiveOptions> => {
810
const {
911
GRAPHILE_SCHEMA,
1012

@@ -34,27 +36,52 @@ export const getGraphQLEnvVars = (env: NodeJS.ProcessEnv = process.env): Partial
3436
DEVSMS_BASE_URL,
3537
} = env;
3638

39+
// Keep this function as a partial env-override parser. Defaults are applied
40+
// before config in constructiveGraphqlDefaults; injecting them here would
41+
// incorrectly let an absent env var overwrite pgpm.json.
42+
const smsRequestTimeoutMs = parseEnvNumber(SMS_REQUEST_TIMEOUT_MS);
43+
const smsDryRun = parseEnvBoolean(SEND_SMS_DRY_RUN);
44+
const hasSmsEnvOverrides = Boolean(
45+
SMS_PROVIDER ||
46+
SMS_SENDER_ID ||
47+
smsRequestTimeoutMs !== undefined ||
48+
smsDryRun !== undefined ||
49+
DEVSMS_BASE_URL
50+
);
51+
3752
return {
3853
graphile: {
3954
...(GRAPHILE_SCHEMA && {
4055
schema: GRAPHILE_SCHEMA.includes(',')
41-
? GRAPHILE_SCHEMA.split(',').map(s => s.trim())
42-
: GRAPHILE_SCHEMA
56+
? GRAPHILE_SCHEMA.split(',').map((s) => s.trim())
57+
: GRAPHILE_SCHEMA,
4358
}),
4459
},
4560
features: {
46-
...(FEATURES_SIMPLE_INFLECTION && { simpleInflection: parseEnvBoolean(FEATURES_SIMPLE_INFLECTION) }),
47-
...(FEATURES_OPPOSITE_BASE_NAMES && { oppositeBaseNames: parseEnvBoolean(FEATURES_OPPOSITE_BASE_NAMES) }),
61+
...(FEATURES_SIMPLE_INFLECTION && {
62+
simpleInflection: parseEnvBoolean(FEATURES_SIMPLE_INFLECTION),
63+
}),
64+
...(FEATURES_OPPOSITE_BASE_NAMES && {
65+
oppositeBaseNames: parseEnvBoolean(FEATURES_OPPOSITE_BASE_NAMES),
66+
}),
4867
...(FEATURES_POSTGIS && { postgis: parseEnvBoolean(FEATURES_POSTGIS) }),
4968
},
5069
api: {
51-
...(API_ENABLE_SERVICES && { enableServicesApi: parseEnvBoolean(API_ENABLE_SERVICES) }),
70+
...(API_ENABLE_SERVICES && {
71+
enableServicesApi: parseEnvBoolean(API_ENABLE_SERVICES),
72+
}),
5273
...(API_IS_PUBLIC && { isPublic: parseEnvBoolean(API_IS_PUBLIC) }),
53-
...(API_EXPOSED_SCHEMAS && { exposedSchemas: API_EXPOSED_SCHEMAS.split(',').map(s => s.trim()) }),
54-
...(API_META_SCHEMAS && { metaSchemas: API_META_SCHEMAS.split(',').map(s => s.trim()) }),
74+
...(API_EXPOSED_SCHEMAS && {
75+
exposedSchemas: API_EXPOSED_SCHEMAS.split(',').map((s) => s.trim()),
76+
}),
77+
...(API_META_SCHEMAS && {
78+
metaSchemas: API_META_SCHEMAS.split(',').map((s) => s.trim()),
79+
}),
5580
...(API_ANON_ROLE && { anonRole: API_ANON_ROLE }),
5681
...(API_ROLE_NAME && { roleName: API_ROLE_NAME }),
57-
...(API_DEFAULT_DATABASE_ID && { defaultDatabaseId: API_DEFAULT_DATABASE_ID }),
82+
...(API_DEFAULT_DATABASE_ID && {
83+
defaultDatabaseId: API_DEFAULT_DATABASE_ID,
84+
}),
5885
},
5986
...((EMBEDDER_PROVIDER || CHAT_PROVIDER) && {
6087
llm: {
@@ -74,18 +101,18 @@ export const getGraphQLEnvVars = (env: NodeJS.ProcessEnv = process.env): Partial
74101
}),
75102
},
76103
}),
77-
...((SMS_PROVIDER || SMS_SENDER_ID || SMS_REQUEST_TIMEOUT_MS || SEND_SMS_DRY_RUN || DEVSMS_BASE_URL) && {
104+
...(hasSmsEnvOverrides && {
78105
sms: {
79106
...(SMS_PROVIDER && { provider: SMS_PROVIDER }),
80107
...(SMS_SENDER_ID && { senderId: SMS_SENDER_ID }),
81-
...(SMS_REQUEST_TIMEOUT_MS && {
82-
requestTimeoutMs: parseEnvNumber(SMS_REQUEST_TIMEOUT_MS)
108+
...(smsRequestTimeoutMs !== undefined && {
109+
requestTimeoutMs: smsRequestTimeoutMs,
83110
}),
84-
...(SEND_SMS_DRY_RUN && { dryRun: parseEnvBoolean(SEND_SMS_DRY_RUN) }),
111+
...(smsDryRun !== undefined && { dryRun: smsDryRun }),
85112
...(DEVSMS_BASE_URL && {
86113
devsms: {
87-
baseUrl: DEVSMS_BASE_URL
88-
}
114+
baseUrl: DEVSMS_BASE_URL,
115+
},
89116
}),
90117
},
91118
}),

‎graphql/types/src/sms.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,13 @@ export interface SmsOptions {
2323
devsms?: DevSmsOptions;
2424
}
2525

26+
/**
27+
* Honest fallbacks for every environment (12factor-env class 1).
28+
*
29+
* These live in the domain-default layer rather than the env parser so config
30+
* files can override them when the corresponding env variables are absent.
31+
*/
2632
export const smsDefaults: SmsOptions = {
2733
requestTimeoutMs: 5000,
28-
dryRun: false
34+
dryRun: false,
2935
};

0 commit comments

Comments
 (0)