feat(cli): add validate subcommand #165

indirection42 · 2024-04-23T08:30:58Z

Resolve #55

Usage:

subway --config configs/config.yml validate
Note: since using subway validate --config configs/config.yml mentioned in #55 will require config being the option of the validate subcommand, I use the above form instead. If the former form is proper, a new run subcommand can be added.

Detailed validations performed

static checks: use garde crate's proc_macro to make validation more extendible. Including:
- endpoints must be websocket URL format
- ensure each method has only one param with inject=true
- ensure each method has only one param with inject=true
dynamic(rely on outside state) checks:
- simple endpoints connection check, just test if TCP handshake is a success

Example Success Output (with exit code 0):

2024-04-17T12:52:26.716236Z  INFO subway::extensions::client: Connected to endpoint: wss://acala-rpc-0.aca-api.network
2024-04-17T12:52:28.811641Z  INFO subway::extensions::client: Connected to endpoint: wss://acala-rpc.dwellir.com

Example Failure Output (with exit code 1):

2024-04-17T12:53:15.457831Z  INFO subway::extensions::client: Connected to endpoint: wss://acala-rpc-0.aca-api.network
2024-04-17T12:53:15.580627Z ERROR subway::extensions::client: Failed to connect to endpoint: wss://example.com, error: Networking or low-level protocol error: Connection rejected with status code: 200

Caused by:
    Connection rejected with status code: 200
2024-04-17T12:53:15.673427Z  INFO subway::extensions::client: Connected to endpoint: wss://acala-rpc.dwellir.com
Error: Unable to connect to all endpoints

Questions:

I found middlewares and extensions are two sets, but they are intersected, should I do checks for the intersection?
Should the validation test be performed in #[cfg(test)] (for developers, it makes sense), or we validate it individually in CI with an individual command?

src/cli.rs

xlc · 2024-04-24T09:27:28Z

src/config/mod.rs

    pub extensions: ExtensionsConfig,
+    // TODO: #[garde(custom(has_matched_extensions(&self.extensions)))]


what is this TODO for?

It's for question 1 which I raised at the beginning of this PR's main comments.

xlc · 2024-04-24T09:28:47Z

src/config/mod.rs

+    }
+
+    #[tokio::test]
+    #[ignore = "This test is not support yet"]


why it is not yet supported?
if it is not yet implemented, then no need to have a not working test here

Oh, this is also related to Question 1 in this PR's main comment. If the question gets clarified, I will fix it.

xlc · 2024-04-24T09:30:36Z

src/config/rpc.rs

+        assert!(test_fn(&invalid_params, &()).is_err());
+        // since test_fn is FnOnce, we need get a new one
+        let test_fn = validate_params_with_name(method_name);
+        assert!(test_fn(&another_invalid_params, &()).is_err());


best to break this into two different tests. and need a test for a valid one

xlc · 2024-04-24T10:27:37Z

Middleware depends on extensions and yes it will be helpful to ensure the required extensions are defined.
We can validate the default config in CI. The main use case is to validate the production config file, which is different for every team.

xlc · 2024-04-26T02:51:01Z

src/config/mod.rs

+    };
+}
+
+define_middleware_extension_mappings! {


this isn't very extensible as we need to define all the mapping here and it is very easy to make mistake when we introduce new extensions or make modifications.

should add a new validate method to the extension and call it instead. in that way the validation and use are in a same file and harder to modify one without modify others. and we should be able to do it in such way to ensure it is not possible to require an extension without validate it.

Yeah, I agree. As you said, to make it not possible to use without validating it, I think the best way is to ensure it via type-safety. The process is like this: first we validate at some entrypoint which typically in the very beginning, then we wrap it into a new-type, and use this validated new-type all over the rest. Everywhere we use the new-type, we know the value has been validated. But it may take lots of changes in the codes, I will try to get a workaround.

let's remove this part for now so it is not blocking this PR and then we can do the extension validation in next PR

…validation

ermalkaleci

can you move testing config files into another directory?

indirection42 · 2024-04-26T11:11:16Z

can you move testing config files into another directory?

Yeah, I felt like doing so. Should I move the config_with_env.yml too?

xlc reviewed Apr 24, 2024

View reviewed changes

indirection42 added 3 commits April 25, 2024 19:37

feat(cli): add validate subcommand

1939a5e

fix(config): fix test.yml

43d7cdd

feat(config): support middleware and extensions mapping validation

200e86f

indirection42 force-pushed the feat-validate-subcommand branch from bf9a7bf to 200e86f Compare April 25, 2024 13:06

xlc requested changes Apr 26, 2024

View reviewed changes

revert(config): remove unergonomic middleware and extension mappings …

a44d3ff

…validation

xlc approved these changes Apr 26, 2024

View reviewed changes

ermalkaleci requested changes Apr 26, 2024

View reviewed changes

refactor(config): move test configs

7dc611c

indirection42 requested a review from ermalkaleci April 28, 2024 02:47

indirection42 merged commit a1628f5 into master May 7, 2024
1 check passed

indirection42 deleted the feat-validate-subcommand branch May 7, 2024 07:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): add validate subcommand #165

feat(cli): add validate subcommand #165

indirection42 commented Apr 23, 2024 •

edited

Loading

xlc Apr 24, 2024

indirection42 Apr 24, 2024

xlc Apr 24, 2024

indirection42 Apr 24, 2024

xlc Apr 24, 2024

xlc commented Apr 24, 2024

xlc Apr 26, 2024

indirection42 Apr 26, 2024 •

edited

Loading

xlc Apr 26, 2024

indirection42 Apr 26, 2024

ermalkaleci left a comment

indirection42 commented Apr 26, 2024 •

edited

Loading

		pub extensions: ExtensionsConfig,
		// TODO: #[garde(custom(has_matched_extensions(&self.extensions)))]

feat(cli): add validate subcommand #165

feat(cli): add validate subcommand #165

Conversation

indirection42 commented Apr 23, 2024 • edited Loading

Usage:

Detailed validations performed

Example Success Output (with exit code 0):

Example Failure Output (with exit code 1):

Questions:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xlc commented Apr 24, 2024

Choose a reason for hiding this comment

indirection42 Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ermalkaleci left a comment

Choose a reason for hiding this comment

indirection42 commented Apr 26, 2024 • edited Loading

indirection42 commented Apr 23, 2024 •

edited

Loading

indirection42 Apr 26, 2024 •

edited

Loading

indirection42 commented Apr 26, 2024 •

edited

Loading