fix: Don't replace malformed appmap.yml #125

zermelo-wisen · 2024-03-08T12:23:14Z

Fixes #80.

dividedmind · 2024-03-14T17:14:58Z

src/config.ts

-  const config = tryOr(() => YAML.parse(readFileSync(path, "utf8")) as unknown);
+  const fileContent = tryOr(() => readFileSync(path, "utf8"));
+  if (!fileContent) return;
+  const config = tryOr(() => YAML.parse(fileContent) as unknown, { malformed: true });


This is a weird approach. Can you try {...} catch { return "malformed" } instead? readConfigFile would return ConfigFile | "malformed" | undefined and you could check for that instead.

Actually, at first, I thought about making it so, but later, I thought it leads to needless changes in surrounding parts and switched to this one. Now I changed it to ConfigFile | "malformed" | undefined return.

dividedmind

I gave this some more thought, and I think it would be good to expand the scope a bit; in particular, this will still lead to replacing the config file on eg. EACCES error, which can be a problem.

Perhaps readConfigFile should just rethrow any exceptions except ENOENT (which would return undefined). The user can then be warned with the specific error message to make it clearer what's going on.

dividedmind · 2024-03-18T12:39:09Z

src/config.ts

@@ -94,12 +97,16 @@ interface ConfigFile {
  name?: string;
  packages?: Package[];
  response_body_max_length?: number;
+  malformed?: boolean;


I think you forgot to remove this.

zermelo-wisen · 2024-03-19T12:39:18Z

I gave this some more thought, and I think it would be good to expand the scope a bit; in particular, this will still lead to replacing the config file on eg. EACCES error, which can be a problem.

Perhaps readConfigFile should just rethrow any exceptions except ENOENT (which would return undefined). The user can then be warned with the specific error message to make it clearer what's going on.

Ok, If they have a config file besides that they don't want it to be replaced, they don't want to use default parameters, too. So we don't go on with the default config if it throws any exception except ENOENT. User will need to resolve the specific error with their config file in order to proceed.

What about exiting when it's malformed too?

dividedmind · 2024-03-26T06:16:21Z

I gave this some more thought, and I think it would be good to expand the scope a bit; in particular, this will still lead to replacing the config file on eg. EACCES error, which can be a problem.
Perhaps readConfigFile should just rethrow any exceptions except ENOENT (which would return undefined). The user can then be warned with the specific error message to make it clearer what's going on.

Ok, If they have a config file besides that they don't want it to be replaced, they don't want to use default parameters, too. So we don't go on with the default config if it throws any exception except ENOENT. User will need to resolve the specific error with their config file in order to proceed.

What about exiting when it's malformed too?

Yeah, perhaps you're right. Maybe we should just let the exceptions propagate and catch ENOENT only in Config constructor. I'm starting to think that will be the least surprising. What do you think?

zermelo-wisen · 2024-03-26T06:34:54Z

I gave this some more thought, and I think it would be good to expand the scope a bit; in particular, this will still lead to replacing the config file on eg. EACCES error, which can be a problem.
Perhaps readConfigFile should just rethrow any exceptions except ENOENT (which would return undefined). The user can then be warned with the specific error message to make it clearer what's going on.

Ok, If they have a config file besides that they don't want it to be replaced, they don't want to use default parameters, too. So we don't go on with the default config if it throws any exception except ENOENT. User will need to resolve the specific error with their config file in order to proceed.
What about exiting when it's malformed too?

Yeah, perhaps you're right. Maybe we should just let the exceptions propagate and catch ENOENT only in Config constructor. I'm starting to think that will be the least surprising. What do you think?

I agree. One point that makes me think is that the user should be given the opportunity to run with defaults if they struggle to fix their config file. For this, maybe we should inform them to remove (backup) or rename their config file to run with defaults. Or we can introduce a flag like --ignore-config-file or --use-defaults, and let the user know they can use these.

dividedmind · 2024-03-26T06:42:16Z

I gave this some more thought, and I think it would be good to expand the scope a bit; in particular, this will still lead to replacing the config file on eg. EACCES error, which can be a problem.
Perhaps readConfigFile should just rethrow any exceptions except ENOENT (which would return undefined). The user can then be warned with the specific error message to make it clearer what's going on.

Ok, If they have a config file besides that they don't want it to be replaced, they don't want to use default parameters, too. So we don't go on with the default config if it throws any exception except ENOENT. User will need to resolve the specific error with their config file in order to proceed.
What about exiting when it's malformed too?

Yeah, perhaps you're right. Maybe we should just let the exceptions propagate and catch ENOENT only in Config constructor. I'm starting to think that will be the least surprising. What do you think?

I agree. One point that makes me think is that the user should be given the opportunity to run with defaults if they struggle to fix their config file. For this, maybe we should inform them to remove (backup) or rename their config file to run with defaults. Or we can introduce a flag like --ignore-config-file or --use-defaults, and let the user know they can use these.

Nah, I don't think the flag is necessary. But I like the idea of explaining the problem and indicating the possible solution, eg. with a message like Error parsing config file at ${path}: ${error.message}.\nYou can remove the file to use the default configuration..

Now I changed it to re-throw when malformed too, with the message above.

dividedmind · 2024-03-27T14:30:15Z

src/config.ts

+    config = YAML.parse(fileContent) as unknown;
+  } catch (exn) {
+    const exnMessage =
+      exn && typeof exn == "object" && "message" in exn && typeof exn.message == "string"


You can just assert(isNativeError(exn)) for simplicity :)

dividedmind · 2024-03-27T14:31:10Z

src/util/tryOr.ts

@@ -1,7 +1,7 @@
-export default function tryOr<T>(fn: () => T): T | undefined {
+export default function tryOr<T, E = undefined>(fn: () => T, elseReturn?: E): T | E | undefined {


I don't think this change is used anymore.

- Use defaults only when there is no config file (ENOENT). - Rethrow every other error thrown from readFileSync (ex: EACCESS). - Rethrow YAML parsing error when appmap.yml is malformed, appending an informative message.

appland-release · 2024-04-03T13:05:30Z

🎉 This PR is included in version 2.19.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

zermelo-wisen requested a review from dividedmind March 8, 2024 12:23

dividedmind requested changes Mar 14, 2024

View reviewed changes

dividedmind force-pushed the main branch from b1aa2b9 to c864d36 Compare March 16, 2024 20:18

zermelo-wisen force-pushed the fix/malformed-appmap-yml-replacement branch 2 times, most recently from 46df1c3 to 62d0093 Compare March 18, 2024 11:35

zermelo-wisen requested a review from dividedmind March 18, 2024 11:55

dividedmind requested changes Mar 18, 2024

View reviewed changes

zermelo-wisen force-pushed the fix/malformed-appmap-yml-replacement branch from 62d0093 to d96cf47 Compare March 19, 2024 13:31

zermelo-wisen force-pushed the fix/malformed-appmap-yml-replacement branch from d96cf47 to d244d0c Compare March 26, 2024 09:51

zermelo-wisen changed the title ~~fix: Don't replace malformed appmap.yml, warn and use defaults~~ fix: Don't replace malformed appmap.yml Mar 26, 2024

zermelo-wisen requested a review from dividedmind March 27, 2024 07:05

dividedmind approved these changes Mar 27, 2024

View reviewed changes

zermelo-wisen force-pushed the fix/malformed-appmap-yml-replacement branch from d244d0c to c8a2caf Compare March 29, 2024 07:26

fix: Don't replace malformed appmap.yml

27d9b44

- Use defaults only when there is no config file (ENOENT). - Rethrow every other error thrown from readFileSync (ex: EACCESS). - Rethrow YAML parsing error when appmap.yml is malformed, appending an informative message.

zermelo-wisen force-pushed the fix/malformed-appmap-yml-replacement branch from c8a2caf to 27d9b44 Compare March 29, 2024 07:28

dividedmind approved these changes Apr 3, 2024

View reviewed changes

dividedmind merged commit 48df24c into main Apr 3, 2024
6 checks passed

dividedmind deleted the fix/malformed-appmap-yml-replacement branch April 3, 2024 13:02

appland-release added the released label Apr 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Don't replace malformed appmap.yml #125

fix: Don't replace malformed appmap.yml #125

zermelo-wisen commented Mar 8, 2024

dividedmind Mar 14, 2024

zermelo-wisen Mar 18, 2024

dividedmind left a comment

dividedmind Mar 18, 2024

zermelo-wisen commented Mar 19, 2024 •

edited

Loading

dividedmind commented Mar 26, 2024

zermelo-wisen commented Mar 26, 2024

dividedmind commented Mar 26, 2024 •

edited by zermelo-wisen

Loading

dividedmind Mar 27, 2024

zermelo-wisen Mar 29, 2024

dividedmind Mar 27, 2024

zermelo-wisen Mar 29, 2024

appland-release commented Apr 3, 2024

		@@ -1,7 +1,7 @@
		export default function tryOr<T>(fn: () => T): T \| undefined {
		export default function tryOr<T, E = undefined>(fn: () => T, elseReturn?: E): T \| E \| undefined {

fix: Don't replace malformed appmap.yml #125

fix: Don't replace malformed appmap.yml #125

Conversation

zermelo-wisen commented Mar 8, 2024

dividedmind Mar 14, 2024

Choose a reason for hiding this comment

zermelo-wisen Mar 18, 2024

Choose a reason for hiding this comment

dividedmind left a comment

Choose a reason for hiding this comment

dividedmind Mar 18, 2024

Choose a reason for hiding this comment

zermelo-wisen commented Mar 19, 2024 • edited Loading

dividedmind commented Mar 26, 2024

zermelo-wisen commented Mar 26, 2024

dividedmind commented Mar 26, 2024 • edited by zermelo-wisen Loading

dividedmind Mar 27, 2024

Choose a reason for hiding this comment

zermelo-wisen Mar 29, 2024

Choose a reason for hiding this comment

dividedmind Mar 27, 2024

Choose a reason for hiding this comment

zermelo-wisen Mar 29, 2024

Choose a reason for hiding this comment

appland-release commented Apr 3, 2024

zermelo-wisen commented Mar 19, 2024 •

edited

Loading

dividedmind commented Mar 26, 2024 •

edited by zermelo-wisen

Loading