Disallow overwrite_key and force_update in Config File

[anipaul2]

fixes #217

[anipaul2]

I have added the complete line and pushed it but only half gets pushed. What maybe the possible error?

[mariocynicys]

save the file maybe 🤔

[anipaul2]

save the file maybe 🤔

Done. Actually, I thought it does it automatically but saw now I have not enabled the feature. Showing now.

[sr-gi]

Don't add this change to the PR, these are entirely unrelated.

[anipaul2]

When I raise a pr, it gets attached automatically and pr gets two commits. So, rebased it and squashed it. What Should I do to make the commit ignore in this pr?

[sr-gi]

Delete it, amend and rebase so it is a single commit

[anipaul2]

Delete it, amend and rebase so it is a single commit

okay.

[sr-gi]

Don't panic. Make the method return a Result and check it on main, if it's an error, eprintln!("{e}") and std::process::exit(1)

[anipaul2]

Don't panic. Make the method return a Result and check it on main, if it's an error, eprintln!("{e}") and std::process::exit(1)

if path_with_options have to updated with result, I have to add Result<(), String> in the function to make the error message work. But the problem is I have to change all the code path function to return a result.

[anipaul2]

Don't panic. Make the method return a Result and check it on main, if it's an error, eprintln!("{e}") and std::process::exit(1)

[mariocynicys]

You should do such sanity checks in config::from_file instead of patch_with_options.

[anipaul2]

You should do such sanity checks in config::from_file instead of patch_with_options.

config::from_file is in main.rs where I don't see any changes can be made regarding this issue. In config.rs, from_file function can be modified to return a Result<T, String> instead of T.

[anipaul2]

I made changes only in the main.rs file because modifying the config.rs file's from_file function would have resulted in errors. The reason for this is that when we removed the generic type T and added Config directly, it caused errors related to the opt variable being linked with overwrite_key and force_update.

Since opt is specific to the command line options, it cannot be directly used in the from_file function, which is responsible for loading the configuration from a file. Therefore, I decided to handle the sanity checks in the main.rs file instead.

[mariocynicys]

You should have these checks inside config.rs.
What you could do is modify the from_filefunction like so:

pub fn from_file<T: Default + serde::de::DeserializeOwned>(path: PathBuf) -> Result<T, String> {
    match std::fs::read(path) {
        Ok(file_content) => match toml::from_slice::<T>(&file_content) {
            Err(e) => {
                Err(format!("Couldn't parse config file: {e}"))
            }
            Ok(config) => {
                // do the sanity checks here.
                if config.overwrite_key {
                    return Err(format!("Overwrite key isn't allowed in the config file."))
                }
                Ok(config)
            }
        },
        Err(_) => Ok(T::default()),
    }
}

This won't work right away, you will need to adapt some stuff in main.rs and probably cli.rs because it uses the same parsing function.

[mariocynicys]

This doesn't belong here.

[anipaul2]

You should have these checks inside config.rs. What you could do is modify the from_filefunction like so:

pub fn from_file<T: Default + serde::de::DeserializeOwned>(path: PathBuf) -> Result<T, String> {
    match std::fs::read(path) {
        Ok(file_content) => match toml::from_slice::<T>(&file_content) {
            Err(e) => {
                Err(format!("Couldn't parse config file: {e}"))
            }
            Ok(config) => {
                // do the sanity checks here.
                if config.overwrite_key {
                    return Err(format!("Overwrite key isn't allowed in the config file."))
                }
                Ok(config)
            }
        },
        Err(_) => Ok(T::default()),
    }
}

This won't work right away, you will need to adapt some stuff in main.rs and probably cli.rs because it uses the same parsing function.

Got it.

[anipaul2]

You should have these checks inside config.rs. What you could do is modify the from_filefunction like so:

pub fn from_file<T: Default + serde::de::DeserializeOwned>(path: PathBuf) -> Result<T, String> {
    match std::fs::read(path) {
        Ok(file_content) => match toml::from_slice::<T>(&file_content) {
            Err(e) => {
                Err(format!("Couldn't parse config file: {e}"))
            }
            Ok(config) => {
                // do the sanity checks here.
                if config.overwrite_key {
                    return Err(format!("Overwrite key isn't allowed in the config file."))
                }
                Ok(config)
            }
        },
        Err(_) => Ok(T::default()),
    }
}

This won't work right away, you will need to adapt some stuff in main.rs and probably cli.rs because it uses the same parsing function.

I've tried implementing the changes you've suggested. Specifically, I updated the from_file function to include checks for overwrite_key. However, I'm encountering an issue. The from_file function is a generic function, which takes any type T that implements Default + serde::de::DeserializeOwned. This means it can't access fields specific to Config like overwrite_key directly, because these fields aren't part of the constraints we've put on T. This seems to lead to a type mismatch. I tried updating main.rs and cli.rs to adapt to these changes as well, but the issue persists. Could you kindly provide some guidance on how I could implement the desired checks while preserving the generic nature of the from_file function?

I am thinking of removing the T and make it config.

[mariocynicys]

I think a good solution is to serialize the config as a serde object and disallowing the fields we want disallowed. @anipaul2

[anipaul2]

I think a good solution is to serialize the config as a serde object and disallowing the fields we want disallowed. @anipaul2

Yes sounds like a reasonable approach.

[anipaul2]

@sr-gi @mariocynicys I have first made the changes in cli.rs, main.rs, and config.rs to disallow overwrite_key and force_update but only skipping the serializing in the flags part does this. Let me know if this approach is wrong but it worked on my end.

[sr-gi]

I don't think what you are doing is right. This is only preventing Coinf from serializing those two items. You could prevent it from deserializing it, which will prevent us from reading that, but I don't think that'll be correct either.

Giving this a closer look, I'm starting to lean toward two different solutions:

Either reworking the Config logic to specify where the data comes from, potentially using an enum, which will help towards #204 (comment), or doing this either with a custom deserializer or in patch_with_options.

I think @anipaul2 has a point in that from_file may not be the best place to handle this, given that is used for both reading from teosd conf and teos-cli conf, and these two params have nothing to do with the latter.

[mariocynicys]

given that is used for both reading from teosd conf and teos-cli conf, and these two params have nothing to do with the latter.

Agreed. Even though these two parameter should just default to None when queried in the cli config. But doesn't make a lot of sense querying it in the first place.

Either reworking the Config logic to specify where the data comes from, potentially using an enum, which will help towards #204 (comment), or doing this either with a custom deserializer or in patch_with_options.

Not sure how the enum would help regarding the comment you linked. I'm thinking now of copying this function over to cli_config and not make it generic anymore. Could this help the issue you linked?

[sr-gi]

given that is used for both reading from teosd conf and teos-cli conf, and these two params have nothing to do with the latter.

Agreed. Even though these two parameter should just default to None when queried in the cli config. But doesn't make a lot of sense querying it in the first place.

Either reworking the Config logic to specify where the data comes from, potentially using an enum, which will help towards #204 (comment), or doing this either with a custom deserializer or in patch_with_options.

Not sure how the enum would help regarding the comment you linked. I'm thinking now of copying this function over to cli_config and not make it generic anymore. Could this help the issue you linked?

This is what I'm thinking about give or take. However, I haven't been able to make the deserialization of ConfigParam work, looks like there's an issue with the trait bounds that I'm still scratching my head about:

master...sr-gi:rust-teos:config-rework

[mariocynicys]

looks like there's an issue with the trait bounds that I'm still scratching my head about

@sr-gi Fixed the compilation issues in this branch. Acking the approach :).
https://github.com/mariocynicys/rust-teos/tree/config-rework-fix

[anipaul2]

@sr-gi @mariocynicys how to approach this issue now? any ideas? I am thinking of making a change in patch_with_options and defining a new struct for overwrite_key and force_update.

[sr-gi]

I think I'd make more sense to fix this one by reworking the config as I was trying to do here: master...sr-gi:rust-teos:config-rework

I haven't had much time lately to finish it, so no worries about this for now, it's not critical, or you can pick up when I left it if you like

[anipaul2]

I think I'd make more sense to fix this one by reworking the config as I was trying to do here: master...sr-gi:rust-teos:config-rework

I haven't had much time lately to finish it, so no worries about this for now, it's not critical, or you can pick up when I left it if you like

okay.