Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible Inaccurate Detection of Existing Runtime Configuration in config.toml #44

Open
tillknuesting opened this issue Mar 14, 2024 · 3 comments
Labels
help wanted Extra attention is needed

Comments

@tillknuesting
Copy link

tillknuesting commented Mar 14, 2024

if strings.Contains(string(data), runtimeName) {

The current implementation checks for the existence of a specific runtimeName in the config.toml file uses a simple substring search. This approach, while straightforward, has several limitations and potential pitfalls:

  1. Partial Matches: The method can yield false positives if runtimeName is a substring of another unrelated configuration or value within the file.
  2. Context Ignorance: The detection does not consider the context or the exact location of runtimeName within the file, leading to inaccurate assessments if the name appears in comments or as part of other keys/values.
  3. Case Sensitivity: The search is case-sensitive, missing occurrences with different capitalizations.
  4. Inefficiency with Large Files: Loading and searching through the entire file content as a string might not be efficient for large configuration files.

Proposed Solution:
Use a proper TOML parser to accurately and efficiently verify the existence of runtimeName within the correct context.

Impact:
Addressing this issue will enhance reliability and efficiency, reducing the risk of incorrect configurations.

@tillknuesting tillknuesting changed the title Inaccurate Detection of Existing Runtime Configuration in config.toml Possible inaccurate Detection of Existing Runtime Configuration in config.toml Mar 14, 2024
@tillknuesting tillknuesting changed the title Possible inaccurate Detection of Existing Runtime Configuration in config.toml Possible Inaccurate Detection of Existing Runtime Configuration in config.toml Mar 14, 2024
@voigt
Copy link
Contributor

voigt commented Mar 15, 2024

I think the reason why a toml parser hasn't been used in first place for adding/removing items is to retain potential comments in the file.

We could definitely challenge this decision. There may be other approaches to keep the comments.

However, parsing toml to check validity might be possible/should be done in any case.

@voigt voigt added the help wanted Extra attention is needed label Mar 17, 2024
@phyrog
Copy link
Collaborator

phyrog commented Mar 19, 2024

1 and 2 are definitely problems we need to solve.

3 is not a problem IMO. We insert the runtimeName in the file, so it should always have the same capitalization that we check for.

4 is also valid, but probably not solved by using a parser that would also have to read the complete file.

@rajatjindal
Copy link
Member

I think the reason why a toml parser hasn't been used in first place for adding/removing items is to retain potential comments in the file.
We could definitely challenge this decision. There may be other approaches to keep the comments.

maybe we parse the file in toml format to confirm if the expected config is there or not, and keep the same mechanism to add the config if it is found to be missing? This way we can retain the comments in the file. (but maybe i am missing something obvious here).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants