Skip to content

Subscription#106

Open
ZohebShaikh wants to merge 10 commits into
mainfrom
subscription
Open

Subscription#106
ZohebShaikh wants to merge 10 commits into
mainfrom
subscription

Conversation

@ZohebShaikh

@ZohebShaikh ZohebShaikh commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

These are the limitation of the tiled server ws

  • External registered tabular data is not yet supported.
  • Other data structures (sparse, awkward) are not yet supported.
  • Arrays can be patched with either extend=True or persist=False, but not both.
  • Deletions of nodes are not yet visible to subscribers.

Currently I can get updates on the root node of creation of new node
and patch updates + creation updates for a particular node

To test the subscription to tiled websocket

  1. docker run -d --rm --name tiled-test-redis -p 6379:6379 docker.io/redis:7-alpine
  2. tiled serve catalog --temp --api-key secret --cache redis://localhost:6379

Todo:

  1. I was not able to pass the API key in the header (Done)
  2. Deserialization logic as in tiled code
  3. tests Done with help from gemini

@ZohebShaikh ZohebShaikh changed the title initial work Subscription Mar 30, 2026
@codecov

codecov Bot commented Mar 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 57.00000% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.91%. Comparing base (a2eb617) to head (74b1af1).

Files with missing lines Patch % Lines
src/handlers.rs 0.00% 32 Missing ⚠️
src/main.rs 0.00% 7 Missing ⚠️
src/clients.rs 90.47% 2 Missing ⚠️
src/model/subscription.rs 94.59% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #106      +/-   ##
==========================================
+ Coverage   49.27%   50.91%   +1.64%     
==========================================
  Files          13       14       +1     
  Lines         621      707      +86     
==========================================
+ Hits          306      360      +54     
- Misses        315      347      +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tpoliaw

tpoliaw commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

I was not able to pass the API key in the header

Possibly related to this, I'm not able to run any subscriptions. Is there a way to get it working with the current state or does the auth stuff need fixing first?

@ZohebShaikh

Copy link
Copy Markdown
Contributor Author

The way I'm testing it is by chaning these lines in the tiled server code to just return "secret"

https://github.com/bluesky/tiled/blob/481cd1844a05f34470c69dc5492809b00365513f/tiled/server/authentication.py#L487

async def get_current_principal_websocket(
    websocket: WebSocket,
    api_key: Optional[str] = Depends(get_api_key_websocket),
    settings: Settings = Depends(get_settings),
    db_factory: Callable[[], Optional[AsyncSession]] = Depends(
        get_database_session_factory
    ),
):
    return "secret"

I tried to get the headers today but could'nt find a way
I think I need to add something like this

@ZohebShaikh ZohebShaikh marked this pull request as ready for review April 24, 2026 11:27
@ZohebShaikh ZohebShaikh requested a review from a team as a code owner April 24, 2026 11:27
@ZohebShaikh ZohebShaikh marked this pull request as draft April 24, 2026 11:29
@ZohebShaikh ZohebShaikh removed the request for review from a team April 24, 2026 11:29
@ZohebShaikh ZohebShaikh marked this pull request as ready for review April 24, 2026 11:32
@ZohebShaikh ZohebShaikh requested a review from a team April 24, 2026 11:32

@tpoliaw tpoliaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The public_address problem and the error reporting are the main changes needed - most of the other comments aren't critical

Comment thread src/model/subscription.rs Outdated
Comment thread src/model/subscription.rs Outdated
Comment thread src/model/subscription.rs Outdated
Comment thread src/model/subscription.rs Outdated
Comment thread src/model/subscription.rs Outdated
Comment thread src/handlers.rs Outdated
Comment thread src/handlers.rs Outdated
Comment thread src/model/subscription.rs Outdated
Comment thread src/handlers.rs Outdated
Comment thread Cargo.toml Outdated
@ZohebShaikh

Copy link
Copy Markdown
Contributor Author

@tpoliaw I have fixed the public address issue and the errors are also reported now.

Think to note : tiled has a new way of doing auth for websocket
The process is tiled lets a websocket connection to be opened if there is no headers and waits for a Auth token etc in the first message. If the first message has a vaild token it starts giving you data/ events.
So if you make a call from glazed without any headers it will look like nothing happening and you are also not getting any errors.

To test if errors are coming try adding the apikey header with a wrong value

For unit tests:-
I think we can create some integration tests rather than creating a mock websocket / mock tiled. It will be more useful and is worth putting effort into

@ZohebShaikh ZohebShaikh requested a review from tpoliaw May 8, 2026 14:23

@tpoliaw tpoliaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The graphiql editor is also failing to get the schema with a CORS error - no idea why

Comment thread src/model/subscription.rs
Comment on lines +182 to +183
.expect("Timeout waiting for headers")
.expect("Channel closed");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: expect messages are usually the condition you expect to be met rather than the error state, so something like .expect("Headers should have be sent after stream is read above") or similar.

Comment thread src/clients.rs
let (ws_stream, _) = match connect_async(request).await {
Ok(ws) => ws,
Err(e) => {
return error!("Failed to connect to WebSocket: {}", e);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is returning () and the user won't see any errors. Could we make the method return either Stream<Item=Result<...>> or Result<Stream<...>> (or even Result<Stream<Item=Result<...>>>) so that the user gets feedback when the connection fails?

Comment thread src/handlers.rs
.protocols(ALL_WEBSOCKET_PROTOCOLS)
.on_upgrade(move |socket| {
GraphQLWebSocket::new(socket, schema.0, protocol)
.on_connection_init(|value| async move {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is value here? Can it be named something more descriptive?

Comment thread src/handlers.rs
Comment on lines +56 to +59
let (endpoint, subscription_endpoint) = (
public_address.join("graphql").unwrap().to_string(),
public_address.join("ws").unwrap().to_string(),
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why merge these into the same statement?

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.

2 participants