7446 document rest api by ca61688 · Pull Request #7582 · gchq/sleeper

ca61688 · 2026-07-01T14:47:33Z

Make sure you have checked all steps below.

Issue

My PR fully resolves the following issues. I've referenced an issue in the PR title, for example "Issue 1234 - My
Feature". Note that before an issue is finished, you can still make a pull request by raising a separate issue
for your progress.
- Resolves Document REST API to add a Sleeper table #7446

Tests

My PR adds the following tests based on our test strategy OR does not need testing for this extremely good reason:
- Manual testing following documentation added

Documentation

In case of new functionality, my PR adds documentation that describes how to use it, or I have linked to a
separate issue for that below.
If I have added new Java code, I have added Javadoc that explains it following our conventions and style.
If I have added or removed any dependencies from the project, I have updated the NOTICES file.

rtjd6554 · 2026-07-03T15:19:38Z

  time
 * `DashboardStack` - to create a CloudWatch dashboard showing recorded metrics
 * `BulkExportStack` - to export a whole table as parquet files
+* `RestApiStack` - for a [REST API](../rest-api/README.md) to interact with the instance over HTTPS. Currently early;


Think we need a better word than "early" here
Suggest in develop, or something along those lines

patchwork01 · 2026-07-06T07:56:38Z

        configBucketStack.grantWrite(grantee);
        tableIndexStack.grantReadWrite(grantee);
-        stateStoreStacks.grantReadWritePartitions(grantee);
+        stateStoreStacks.grantReadWriteAllFilesAndPartitions(grantee);


Why was this changed? I don't think editing the table configuration involves files?

When I was testing the rest api the add table wasn't working and needed this change

I think this needs raising as a separate bug.

If the API isn't working we'll want to include the bug fix in the changelog. If we do it in this PR we're likely to miss it when writing the changelog, and we wouldn't have a record that the bug existed.

We could also do with identifying when the bug first occurred, and how it broke.

It's not clear from this PR what the bug is, or why this would be needed. Do you want to clarify it here or just raise a new issue?

Raised #7677

patchwork01 · 2026-07-06T07:58:24Z

-        instanceProperties = requireNonNull(builder.instanceProperties);
-        addTable = requireNonNull(builder.addTable);
+        // Fields may be null when this route is instantiated by the OpenAPI doc generator, which only invokes the
+        // openApi* methods below. handle() will fail with an NPE if the route is dispatched a request without them.


Why is it necessary to instantiate this class in order to generate the documentation? I'm not sure that needs to be the case?

Because the methods for generating the openApi doc are instance methods and need to be so that the Generator can loop through all routes calling them.

Is there another way you could design it?

I think it feels clunky to support two different modes of instantiation. It feels misleading that some of the fields can be null, when they are required to be non-null for the class to work. It makes the class rather big. The OpenAPI definition isn't actually derived from anything in this class, so I think it doesn't look like it really needs to be all one class?

ca61688 · 2026-07-14T10:33:10Z

Ready for review

patchwork01

I also commented on the following previous threads:

#7582 (comment)
#7582 (comment)

patchwork01 · 2026-07-14T11:18:26Z

 `sendToIngestBatcher.sh`.

 ### Add table
+#### Using the REST API


Maybe it would be best to cover the REST API in a single place? A user will know whether they want to use the API or the scripts, and the documentation will be harder to read if it switches between the two.

That's exactly why I initially had the REST API section separated out which you asked me to change

I'm not sure what you mean, I don't see a comment where I asked for that. When it was separate I commented that the way it was structured didn't fit with our existing documentation, I didn't suggest a particular change - #7582 (comment).

I think the problem here is there's a lot going on in this PR/issue, including the OpenAPI definitions and the bug fix, as well as the docs changes.

Here's a link to this thread if you're reading this in the review: #7582 (comment)

patchwork01 · 2026-07-14T11:18:48Z

+## REST API
+
+Sleeper has an optional REST API for interacting with a deployed instance over HTTPS. Only "add table" is available
+today; see the [REST API documentation](rest-api/README.md) for deployment, authentication, and endpoint details. The


This link is broken.

It looks like the link has been removed, we probably still need a link to the documentation from here?

Here's a link to the original thread: #7582 (comment)

patchwork01

I also commented on the thread on the bug fix: #7582 (comment)

patchwork01 · 2026-07-15T11:20:35Z

-        instanceProperties = requireNonNull(builder.instanceProperties);
-        addTable = requireNonNull(builder.addTable);
+        // Fields may be null when this route is instantiated by the OpenAPI doc generator, which only invokes the
+        // openApi* methods below. handle() will fail with an NPE if the route is dispatched a request without them.


I think it feels clunky to support two different modes of instantiation. It feels misleading that some of the fields can be null, when they are required to be non-null for the class to work. It makes the class rather big. The OpenAPI definition isn't actually derived from anything in this class, so I think it doesn't look like it really needs to be all one class?

patchwork01 · 2026-07-15T11:22:39Z

+    }
+
+    @Override
+    public Operation openApiOperation() {


I think these OpenAPI definitions seem like a lot of overhead to require every time we add a new route. It also looks like a lot of work to keep them correct, and I can't see that being practical.

Everywhere I've seen this used, either source code was auto-generated from an OpenAPI definition, or the OpenAPI definition was derived from the source code for the actual endpoints. With this setup, it doesn't look like there's any check that these definitions are accurate? It also doesn't look like it's used for anything specific?

Does this follow AWS' recommendations for documentation of API Gateway REST APIs?

patchwork01 · 2026-07-15T11:31:25Z

+| Field         | Type                          | Required | Description                                                                                                                                                                                              |
+| ------------- | ----------------------------- | -------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `properties`  | object of string → string     | Yes      | Sleeper [table properties](../property-master.md). Must include `sleeper.table.name`; other properties fall back to instance defaults.                                                             |
+| `schema`      | object                        | Yes      | The [Sleeper schema](../schema.md) for the new table. Any shape accepted by `SchemaSerDe` is accepted here.                                                                                        |


I don't think we can reference SchemaSerDe here, that's the source code and not user-facing. The schema format is documented in the linked file.

patchwork01 · 2026-07-15T11:34:10Z

+JSON
+```
+
+The instance property lookup above shells out to the config bucket; if you already have the URL to


I think the use of "shells out" reads a bit oddly when we're already in a shell. The important bit is it retrieves the properties file from the config bucket.

patchwork01 · 2026-07-15T11:37:39Z

 `sendToIngestBatcher.sh`.

 ### Add table
+#### Using the REST API


I'm not sure what you mean, I don't see a comment where I asked for that. When it was separate I commented that the way it was structured didn't fit with our existing documentation, I didn't suggest a particular change - #7582 (comment).

I think the problem here is there's a lot going on in this PR/issue, including the OpenAPI definitions and the bug fix, as well as the docs changes.

Here's a link to this thread if you're reading this in the review: #7582 (comment)

patchwork01 · 2026-07-15T11:46:34Z

+## REST API
+
+Sleeper has an optional REST API for interacting with a deployed instance over HTTPS. Only "add table" is available
+today; see the [REST API documentation](rest-api/README.md) for deployment, authentication, and endpoint details. The


It looks like the link has been removed, we probably still need a link to the documentation from here?

Here's a link to the original thread: #7582 (comment)

ca61688 added 3 commits July 1, 2026 12:22

7446: Initial push for documentation

54b045f

7446: Add documentation for REST APi and add tables endpoint

0adc524

7446: Update release-process to update openapi yaml if required

6a50475

ca61688 linked an issue Jul 1, 2026 that may be closed by this pull request

Document REST API to add a Sleeper table #7446

Open

ca61688 added 8 commits July 1, 2026 15:49

7446: Merge develop

4c06cdc

7446: Update REST API to give correct permissions for add table flow

f9e4592

7466: Update rest-api documentation

4c0ca60

7446: Add ability to auto generate openapi yaml

ad1a4b7

7446: Regenerate openapi yaml

1a45b9a

7446: Make generate apischema deterministic

ac469c2

Merge branch 'develop' into 7446-document-rest-api

9a10126

7446: Update Notice file

177baef

ca61688 force-pushed the 7446-document-rest-api branch from ab78686 to 177baef Compare July 3, 2026 10:44

ca61688 added 3 commits July 3, 2026 12:01

7446: Fix REST API Stack IT

67e4c83

7446: Add missing javadoc to AddTableRoute

b8867bc

7446: Fix checkstyle issue

3a83458

ca61688 marked this pull request as ready for review July 3, 2026 11:59

ca61688 added the needs-reviewer Pull requests that need a reviewer to be assigned label Jul 3, 2026

rtjd6554 reviewed Jul 3, 2026

View reviewed changes

ca61688 added 3 commits July 3, 2026 16:37

7446: Improve rest-api documentation

b73a684

Merge branch 'develop' into 7446-document-rest-api

5dda4eb

Merge branch 'develop' into 7446-document-rest-api

a3b6521

patchwork01 requested changes Jul 6, 2026

View reviewed changes

patchwork01 removed the needs-reviewer Pull requests that need a reviewer to be assigned label Jul 6, 2026

patchwork01 assigned ca61688 Jul 6, 2026

ca61688 added 4 commits July 14, 2026 09:41

Merge branch 'develop' into 7446-document-rest-api

765dc2f

7446: Remove redundant rest api documentation

0aa168a

7446: Move GenerateApiSpec into rest-api module

e546bd2

7446: Move GenerateApiSpec into rest-api module

7b2513c

ca61688 assigned patchwork01 Jul 14, 2026

ca61688 added the needs-reviewer Pull requests that need a reviewer to be assigned label Jul 14, 2026

ca61688 assigned patchwork01 and unassigned patchwork01 and ca61688 Jul 14, 2026

patchwork01 reviewed Jul 14, 2026

View reviewed changes

patchwork01 assigned ca61688 and unassigned patchwork01 Jul 14, 2026

patchwork01 removed the needs-reviewer Pull requests that need a reviewer to be assigned label Jul 14, 2026

patchwork01 reviewed Jul 14, 2026

View reviewed changes

Comment thread docs/usage/rest-api/add-table.md Outdated

ca61688 added 3 commits July 15, 2026 08:44

7446: Add GenerateOpenApiSpec to git hub workflows check

577cd34

7446: Improve rest api documentation

0e268d6

Merge branch 'develop' into 7446-document-rest-api

ef31c31

ca61688 removed their assignment Jul 15, 2026

ca61688 added the needs-reviewer Pull requests that need a reviewer to be assigned label Jul 15, 2026

patchwork01 reviewed Jul 15, 2026

View reviewed changes

patchwork01 removed the needs-reviewer Pull requests that need a reviewer to be assigned label Jul 15, 2026

patchwork01 assigned ca61688 Jul 15, 2026

ca61688 and others added 2 commits July 15, 2026 14:20

7446: Remove bugfix so it can be done in seperate ticket

8ef057d

Update approvals

fdee75f

Uh oh!

Conversation

ca61688 commented Jul 1, 2026

Issue

Tests

Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patchwork01 Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ca61688 commented Jul 14, 2026

Uh oh!

patchwork01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patchwork01 Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patchwork01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patchwork01 Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

patchwork01 Jul 15, 2026 •

edited

Loading

patchwork01 Jul 15, 2026 •

edited

Loading

patchwork01 Jul 15, 2026 •

edited

Loading