Skip to content

Add KeetaNetCombinedAnchorHTTPServer to merge routes from multiple servers#267

Draft
Copilot wants to merge 4 commits intomainfrom
copilot/combine-keetanet-anchor-routes
Draft

Add KeetaNetCombinedAnchorHTTPServer to merge routes from multiple servers#267
Copilot wants to merge 4 commits intomainfrom
copilot/combine-keetanet-anchor-routes

Conversation

Copy link
Copy Markdown

Copilot AI commented Apr 16, 2026

  • Modify url getter to allow reading a statically-set URL (string/URL instance) even when the server is not started
  • Remove pre-existing // @ts-ignore in constructor by changing #url / url setter from polymorphic this callback type to explicit KeetaNetAnchorHTTPServer callback type
  • Add protected get config() to KeetaNetAnchorHTTPServer so subclasses have a single source of truth
  • Add protected buildRoutes() method to KeetaNetAnchorHTTPServer (delegates to initRoutes(this.#config))
  • Add KeetaAnchorCombinedHTTPServerConfig interface
  • Add KeetaNetCombinedAnchorHTTPServer class:
    • No #children field — uses this.config.servers (single source of truth via protected getter)
    • initRoutes uses config.servers parameter
    • start() uses this.config.servers for URL propagation
    • start() validates that any child with a pre-set URL matches the combined server's URL, re-throwing unknown errors from the URL getter
  • Add tests: combining routes, 404 for missing routes, conflict resolution (later child wins), URL propagation, conflicting child URL throws

Copilot AI changed the title [WIP] Add functionality to combine routes in KeetaNetAnchorHTTPServer Add KeetaNetCombinedAnchorHTTPServer to merge routes from multiple servers Apr 16, 2026
Copilot AI requested a review from ezraripps April 16, 2026 22:00
Comment thread src/lib/http-server/index.ts Outdated
Comment on lines +693 to +701
/**
* Static helper that allows KeetaNetCombinedAnchorHTTPServer (and other
* subclasses) to invoke buildRoutes() on an arbitrary
* KeetaNetAnchorHTTPServer instance. Protected access is valid here
* because we are inside the KeetaNetAnchorHTTPServer class body.
*/
protected static callBuildRoutes(server: KeetaNetAnchorHTTPServer): Promise<Routes> {
return(server.buildRoutes());
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should not be needed, if it is needed for testing you can access via server'buildRoutes' in tests as TS allows bypassing private flag that way, or by creating a class that overrides that method.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed callBuildRoutes. initRoutes now calls buildRoutes() via bracket notation directly on each child: (child as unknown as { buildRoutes(): Promise<Routes> }).buildRoutes() (ac464cb).

Comment thread src/lib/http-server/index.ts Outdated

constructor(config: KeetaAnchorCombinedHTTPServerConfig) {
super(config);
this.#children = [...config.servers];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why set this here then ignore it in initRoutes, it should probably just be used in initRoutes, if not we need to verify that both values are the same

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ac464cbinitRoutes now uses the config parameter directly (config.servers) instead of this.#children. The #children field is kept only for the start() override where the base class's private #config isn't accessible.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should figure out a better way than having two sources of truth

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3973da7. Added protected get config(): ConfigType to the base class and removed the #children field entirely. Both initRoutes and start() now read from this.config.servers — one source of truth.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3973da7 — removed #children and added protected get config() on the base class. Both initRoutes and start() now use this.config.servers.

*/
const url = this.url;
for (const child of this.#children) {
child.url = url;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the child already has a URL set? We should ensure they either do not or it is the same.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added validation in ac464cb. Before propagating the URL, start() now tries to read each child's existing URL. If the child already has a static URL set and it differs from the combined server's URL, it throws: Error: Child server url "..." does not match combined server url "...". A test for this case is also added.

Comment thread src/lib/http-server/index.ts Outdated
await super.start();

/*
* Propagate the combined server's URL to all child server
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we perform this validation here, or somewhere else?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validation happens in start() after super.start() — that's the earliest point where the combined server's URL is known (it depends on the port assigned by the OS when no static URL is configured). For the common case of a static string/URL config, we could check earlier, but doing it uniformly in start() keeps the logic in one place.

Comment on lines +689 to +691
protected buildRoutes(): Promise<Routes> {
return(this.initRoutes(this.#config));
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have a method that overrides initRoutes? It does the same thing

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buildRoutes() is a protected proxy that packages initRoutes(this.#config) into a no-argument call. It's needed because initRoutes is generic over ConfigType — you can't call it on an arbitrary child instance from outside the class without knowing (or storing) its config type. The method allows KeetaNetCombinedAnchorHTTPServer to invoke the correct initRoutes on each child via bracket notation without a cast that loses type information.

…utes, validate child URLs

Agent-Logs-Url: https://github.com/KeetaNetwork/anchor/sessions/f2463b59-588b-4959-99c0-b9e5ac7293c2

Co-authored-by: ezraripps <19670988+ezraripps@users.noreply.github.com>
Copilot AI requested a review from ezraripps April 17, 2026 22:14
Comment thread src/lib/http-server/index.ts Outdated
const retval = urlObj.toString();

return(retval);
return(urlObj.toString());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were incidental style cleanups made during the URL getter restructuring (inlining const retval = ...). Reverted in 3973da7 to keep the diff focused on the functional change.

Comment thread src/lib/http-server/index.ts Outdated
newURLObj.search = '';

const retval = newURLObj.toString();
return(newURLObj.toString());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above — incidental style change, reverted in 3973da7.

Comment thread src/lib/http-server/index.ts Outdated

constructor(config: KeetaAnchorCombinedHTTPServerConfig) {
super(config);
this.#children = [...config.servers];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should figure out a better way than having two sources of truth

try {
childURL = child.url;
} catch {
/* child has no static URL set; the URL will be propagated below */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this throw? If it does, should we check the specific error and throw unknown?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The url getter throws new Error('Server not started') when no static URL is set and the server hasn't started. Fixed in 3973da7: the catch now explicitly re-throws anything that isn't that specific error, so unexpected errors are not silently swallowed.

@sonarqubecloud
Copy link
Copy Markdown

…revert cosmetic url changes

Agent-Logs-Url: https://github.com/KeetaNetwork/anchor/sessions/0bd3f93f-6749-464f-867d-98a392a18242

Co-authored-by: ezraripps <19670988+ezraripps@users.noreply.github.com>
Copilot AI requested a review from ezraripps April 17, 2026 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

There should be a way to have multiple KeetaNetAnchorHTTPServer's routes combined

2 participants