Fixes to INI parsing incomplete

[gdavison]

Describe the bug

The PR #2359 fixes some parsing issues with INI files introduced in github.com/aws/aws-sdk-go-v2/internal/ini v1.5. However, the following errors still occur

1. Leading newline plus leading whitespace

	[default]
	region = us-west-2

This will fail to parse silently. The initial token returned by lexer.Tokenize is a TokenLit with the value "\n\t[default]\n\tregion = us-west-2"

2. Subsequent profile is indented

[default]
region = us-west-2

	[profile example]
	region = us-east-1

This will fail to parse silently, but return a config.SharedConfigProfileNotExistError. The corresponding token returned by lexer.Tokenize is a TokenLit with the value "\n\t[profile test]\n\tregion = us-east-1"

Expected Behavior

Both cases should return parsing errors

Current Behavior

Parsing fails silently. In the second case, it returns config.SharedConfigProfileNotExistError instead of a parsing error.

Reproduction Steps

Run tests using the sample INI files above

Possible Solution

No response

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

github.com/aws/aws-sdk-go-v2/config@3db3b148a440
github.com/aws/aws-sdk-go-v2/internal/ini@3db3b148a440

Compiler and Version used

go version go1.21.0 darwin/arm64

Operating System and version

macOS 13.4.1

[lucix-aws]

@gdavison Thanks for the continued engagement on this. At this point I am convinced we need to move forward with an overhaul of the parsing implementation in general.

To provide some context - the implementor of the existing parser is no longer on the project, and with a more or less complete lack of documentation all institutional knowledge of this implementation was lost. At the time of adding support for endpoint URL config, the implementing SDE and myself determined that the "syntax tree" could not be reasonably extended to handle sub-property support required for the feature. We instead chose to handle this at the tokenize stage which has clearly introduced a handful of nasty regressions as you've observed.

To wit I've nuked the old implementation and introduced a greatly simplified line-based version in #2365. The main benefit of the new implementation is to get rid of the frankly asinine behavior of silently dropping entire sections on parse failures, instead we will simply ignore lines that appear in an invalid context and move on.

I've also made the decision to allow leading whitespace in profile declarations, basically the goal being to never risk dropping an otherwise valid section of properties if we can avoid it. This is technically a behavioral break (e.g. property continuation lines that matched profile declaration syntax will instead now be recognized as profiles) but I believe it's the most sensible way forward here.

The behavioral change of not supporting leading whitespace in properties will have to remain EDIT: it actually won't. I've made a change to handle these in context, if a property with leading whitespace (which we'll tokenize as a sub-property) is either at the beginning of a section or the key that precedes it isn't empty, we'll just treat it as a regular property. This combined with allowing leading whitespace on profiles should restore us to a state equivalent to what we were in before last week's change.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.