fix: remove duplicate reads in the static discovery subcommand to avoid reading past EOF when using stdin

[Wafffle77]

Pull Request Template

Thank you for your contribution! Please ensure the following before submitting:

Checklist

• My code follows the style guidelines of this project
• I have added/updated comments where needed
• I have added tests that prove my fix is effective or my feature works
• I have run make test (or equivalent) locally and all tests pass
• DCO Sign-off: All commits are signed off (git commit -s) with my real name and email
• REUSE Compliance:
  • Each new/modified source file has SPDX copyright and license headers
  • Any non-commentable files include a <filename>.license sidecar
  • All referenced licenses are present in the LICENSES/ directory

Description

Fixes an issue where some subcommands (tested with ochami discover static) would attempt to read a data file multiple times. If the data file was stdin, this would result in the file being read successfully once and subsequent reads would be empty. This commit patches the internal io.ReadStdin() to buffer its first successful read from stdin (Full read until EOF, not just a single Read() call) and return the buffered data on subsequent calls. This may break anything that expects this behavior from io.ReadStdin(), but I didn't see anything that relied on it and all the tests came back fine.

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

---

For more info, see Contributing Guidelines.

[synackd]

Good catch! I've examined the code and can see the duplicate payload handling.

I'm a bit hesitant to introduce a global variable here and I think the better solution might be to deduplicate the cli.HandlePayload()/cli.HandlePayloadStdin() calls. For context, these functions appear in both blocks of the if useDeprecatedFormat {} else {} block. [1] [2] It also appears in the discoverStaticDeprecatedFormat() function [3], which is called before the if block.

What I propose is putting the

// Read data from file or stdin
nodes := discover.NodeListDeprecated{}
if cmd.Flag("data").Changed {
	cli.HandlePayload(cmd, &nodes)
} else {
	cli.HandlePayloadStdin(cmd, &nodes)
}

block before the if useDeprecatedFormat {} else {} block and remove it everywhere else.

We would have to unmarshal the data into a map[string]interface{} or, more robustly, an interface{}, perform the deprecation check, then convert the data into the proper data structure (likely through format.MarshalData() into format.UnmarshalData()) to be used by the appropriate discovery method (deprecated versus modern).

On the other hand, it might be time to remove the deprecated discovery format code, since it was deprecated in v0.6.0 (#63). This would eliminate this issue entirely while slimming the codebase.

[Wafffle77]

I think refactoring/removing the deprecated discovery is the best option, but I also wanted to account for any other duplicate reads in the code, since AFAIK there isn't any spec on whether or not duplicate reads should be allowed. It might be a good idea as well to add an error to the ReadStdin function if it gets called more than once per invocation (though that also may require a global variable unless we close stdin when we're done)

[Wafffle77]

I've removed the global stdin buffer variable in favor of deduplicating the stdin reads. Long term it might be a good idea to refactor the data/payload system to make it a bit harder for duplicate reads to happen.

[synackd]

Looks better. Left a comment about some small changes I think would improve this patchset.

[synackd]

Can we use format.UnmarshalData() and format.MarshalData() here instead?

Also, can we be more generic with the discoveryData type and use an interface{}? See below for an example using an interface{} with both functions:

ochami/pkg/client/client.go

Lines 360 to 365 in e7536ed

	var v interface{}
	if err := format.UnmarshalData(data, &v, inFormat); err != nil {
	return nil, fmt.Errorf("failed for convert bytes to HTTP body: %w", err)
	}
	b, err := format.MarshalData(v, format.DataFormatJson)

[synackd]

Any reason these are commented out?

[synackd]

Updated title to follow Conventional Commit formatting for the future squash merge commit message.