Allow empty page_token for get_tree

[amonshiz]

Description

Fixes issue #1339

The RemoteExecution v2 API GetTreeRequest does not require that the page_token value be a non-empty string (or exist) only that it should only exist when the client wants the resume/offset the response based on a prior GetTreeResponse.next_page_token. The original implementation of the get_tree functionality (#905) requires that there always be a value in the page_token field.

The reference implementation in buildfarm treats an empty page_token as an initial request which seems entirely reasonable.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Added new unit tests.

Follow details in #1339 but run a local build of NativeLink using the changes in this PR and verify that the downloaded content exactly matches the uploaded content and there were no errors or warnings from NativeLink.

Checklist

• Tests added/amended
• bazel test //... passes locally
• PR is contained in a single commit, using git amend see some docs

---

This change is 

[CLAassistant]

All committers have signed the CLA.

[allada]

Awesome, thank you very much. Looks good just a few minor comments (not related to primary code, just test hygiene). 😃

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained, and all files reviewed, and pending CI: license/cla, and 3 discussions need to be resolved

---

nativelink-service/tests/cas_server_test.rs line 388 at r1 (raw file):

    // First verify that using an empty page token is treated as if the client had sent the root
    // digest.
    let raw_response = cas_server

nit: Can we put all this new code tabbed in one and scoped inside { /* your code here */ }?

I like to do this for readability to know what code is related to what scope, especially for tests.

(might be good to scope the remaining code now too.

---

nativelink-service/tests/cas_server_test.rs line 397 at r1 (raw file):

        }))
        .await;
    assert!(raw_response.is_ok());

nit: You don't need this, since assert is just a panic and the below .unwrap() will panic anyway if it's not ok.

---

nativelink-service/tests/cas_server_test.rs line 471 at r1 (raw file):

    // First, verify that an empty initial page token is treated as if the client had sent the
    // root digest and respects the page size.
    let raw_response = cas_server

ditto about scoping.

[amonshiz]

Reviewable status: 0 of 2 LGTMs obtained, and 1 of 2 files reviewed, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04 / stable, windows-2022 / stable, and 1 discussions need to be resolved

---

nativelink-service/tests/cas_server_test.rs line 471 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

ditto about scoping.

Done.

[allada]

Reviewable status: 1 of 2 LGTMs obtained, and 1 of 2 files reviewed

[allada]

+@adam-singer or +@aaronmondal for second approval

Reviewable status: 0 of 2 LGTMs obtained, and 1 of 2 files reviewed (waiting on @aaronmondal and @adam-singer)

[adam-singer]

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained, and all files reviewed (waiting on @aaronmondal)

[allada]

-@aaronmondal

Reviewable status: complete! 2 of 2 LGTMs obtained, and all files reviewed