Adarsh-Me commented on PR #3192: URL: https://github.com/apache/hugegraph/pull/3192#issuecomment-5798638213
All eight reproduced before touching anything, and every fix was reverted to confirm its own test goes red. Pushed in 5afbb4a; master merged in 53a5edb (`mergeable: true` again -- #3194 and the startup-timeout change had landed in the same files). You were right on all eight. Table of what each one was, measured against `java.util.Properties` where the question was properties grammar: | thread | what it was | at b8801a6 | now | |---|---|---|---| | [`r4083528783`](https://github.com/apache/hugegraph/pull/3192#discussion_r4083528783) | `authentication: {} # authenticator: X` counted as named because the scanner matched the comment text | `named` | `nameless` | | [`r4083528796`](https://github.com/apache/hugegraph/pull/3192#discussion_r4083528796) | a column-zero comment line ended the mapping, refusing a valid deployment | `nameless` | `named` | | [`r4083528809`](https://github.com/apache/hugegraph/pull/3192#discussion_r4083528809) | authenticator accepted at any depth, so `authentication.config.authenticator` counted | `named` | `nameless` | | [`r4083528814`](https://github.com/apache/hugegraph/pull/3192#discussion_r4083528814) | `check_auth_sides` only ran inside the `PASSWORD` branch | exit 0, server starts | exit 1, refused before init-store | | [`r4083528824`](https://github.com/apache/hugegraph/pull/3192#discussion_r4083528824) | `gremlin\u002egraph` matched no grep pattern, factory left unwrapped | `org.apache.hugegraph.HugeFactory` | `HugeFactoryAuthProxy` | | [`r4083528831`](https://github.com/apache/hugegraph/pull/3192#discussion_r4083528831) | a failed yaml append on a writable REST file still exited 0 | exit 0, half-configured | exit 1 | | [`r4083528843`](https://github.com/apache/hugegraph/pull/3192#discussion_r4083528843) | bare-CR file read as one record; rewriting its first key dropped every later one | `auth.authenticator` gone | read and survives | | [`r4083528855`](https://github.com/apache/hugegraph/pull/3192#discussion_r4083528855) | form feed not treated as separator whitespace | key invisible | parsed as Java sees it | The first, second and third were the same root cause and the worst of them: two of the three produced a false `named`, which is a passed parity check with Gremlin on `AllowAllAuthenticator`. **properties/yaml readers** - `props.awk` now splits records on `\r\n`, `\n` and `\r` and remembers each line's terminator, so a rewrite replays untouched bytes. Your `r4083528843` description was exactly right, and the rewrite is worse than "drop every later key" in one respect worth naming: `auth.authenticator` is usually the later key, so the surviving file is the unauthenticated one. - `\f` is now separator whitespace on both sides of the separator, in the leading-whitespace strip and in the comment/blank-line test. - The yaml reader moved out of the shell into `yamlscan.awk`. It answers about the mapping, not the text: column-zero mapping only, direct child only, in block and flow form alike (a flow key counts at depth one, so `{config: {authenticator: X}}` does not), comments stripped outside quotes, and a direct `authenticator` with an empty/`null`/`~` value treated as nameless rather than named. The last one is not in your list -- it is the same fail-open shape as `r4083528783`, so I closed it while I was in there. - `enable-auth.sh` reads and writes through `props.awk` instead of grep/sed. The embedded-`CR` workaround the old pattern needed goes with it, since the reader now handles terminators. **One packaging change to look at:** `props.awk` moved from `docker/` to `src/assembly/static/bin/`, because `bin/enable-auth.sh` now needs it and the tarball does not ship `docker/`. That directory's assembly fileSet (`<include>*</include>`) picks it up with no descriptor change, and the image gets it from the same place it gets every other `bin/` file, so the `COPY .../props.awk .` line added earlier in this PR is gone. `yamlscan.awk` stays in `docker/` with an explicit COPY -- only the entrypoint uses it. **Two smaller changes the review pushed me into:** `PROPS_MODE=has`, which answers "is this key defined" without confusing `auth.authenticator=` (present, empty) with absent -- appending a default on top of an empty first definition would have left the empty one in force; and exit status 2 now means an error while 1 means absent, so a caller wearing `errexit` cannot read an unreadable file as "not there". **Verification** - `props.awk` against real `java.util.Properties` (javac/java 17) over 15 file shapes -- LF, CRLF, CR-only, mixed, no-final-newline, `\f` on either side of the separator, `\u002e` and `\.` in keys, continuations, indentation, `:`/whitespace separators. Two assertions per shape: every property Java sees must be visible to the reader with the same value, and rewriting one key must leave every other key intact and the key set unchanged. **21 failing assertions at b8801a6, 0 now.** - `yamlscan.awk` over 21 yaml shapes (the three findings, the positive forms a wrong reader must keep accepting, and the negatives). - The eight findings as a standalone before/after table: 8/8 fail at b8801a6, 8/8 pass now. - `test-docker-entrypoint.sh` and `docker-entrypoint-test.sh` both green on the merged tree, which includes the startup-timeout cases from #3187. **What did not run here, so please read the above with this attached:** no Linux container on this host, so **mawk never executed** -- the image's `/usr/bin/awk` is mawk on jammy, and I kept the new awk to `substr`/`index`/`match`, no `RT`, no regex `RS`, no `gensub`, and `gawk --posix --lint` is clean apart from the parameter-shadowing warnings this file already had. The Java oracle is 17, not the 11 the image runs; the grammar cases here are identical between them, but that is reasoning, not a measurement. snakeyaml was never run either, so the yaml rules are from the spec plus a written trace, not from the parser that will actually read these files. Three assertions in the unit suite -- CRLF byte preservation, config-mode preservation, symlinked config -- cannot be evaluated on a Windows host at all (MSYS `gawk` drops the CR of a CRLF pair on read, `chmod` does not affect what `stat` reports, `ln -s` copies), so they are gated behind host probes that print a skip line rather th an pass quietly; they will genuinely run in CI. CI at 53a5edb: six workflows registered, all `completed / action_required` -- so none of them have run, and nothing here is green. A maintainer with write access still needs to approve the pending deployments; `mergeable_state` is `blocked` on that, not on anything in the diff. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
