Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(torii-graphql): event messages update & subscription #2227

Merged
merged 12 commits into from
Aug 13, 2024

Conversation

Larkooo
Copy link
Collaborator

@Larkooo Larkooo commented Jul 29, 2024

Wrap up the work to add event messages for graphql & expose subscription to event messages

Summary by CodeRabbit

  • New Features

    • Subscription mechanism enhanced to handle a broader range of entities with the new "entityUpdated" field.
    • New field eventMessage added to the ModelDataObject, improving GraphQL API data retrieval.
    • Enhanced flexibility in data retrieval mechanisms through dynamic specification of entity ID columns and additional namespace field.
  • Bug Fixes

    • Consistent date formatting implemented across entity attributes, improving data presentation.
  • Refactor

    • Updated data handling to work with a more generalized Entity type, improving extensibility and maintainability.
    • Refined data processing logic for better accuracy in value mappings.
    • Removal of the model_data_recursive_query function to simplify data retrieval processes.
    • Improved test reliability by replacing fixed sleep durations with transaction confirmation waits.

Copy link

coderabbitai bot commented Jul 29, 2024

Walkthrough

Ohayo, sensei! The recent updates significantly enhance entity management flexibility across our codebase. The subscription field is now called entityUpdated, broadening its scope. Key functions have been adapted to utilize the more general Entity type, improving operational consistency. These revisions promote maintainability and set the groundwork for future enhancements, resulting in a streamlined and efficient system.

Changes

Files Change Summary
crates/torii/graphql/src/object/event_message.rs Centralized date-time formatting with a DATETIME_FORMAT constant, updated model_union_field to fetch a namespace, and removed model_data_recursive_query.
crates/torii/graphql/src/object/model_data.rs Added a new eventMessage field and the event_message_field() function for better data retrieval, along with clarity improvements.
crates/torii/graphql/src/constants.rs Introduced EVENT_MESSAGE_ID_COLUMN constant for enhanced metadata handling related to event messages.
crates/torii/graphql/src/object/entity.rs Updated model_data_recursive_query to accept entity_id_column, allowing for more flexible queries.
crates/torii/graphql/src/query/mod.rs Improved value_mapping_from_row to handle multiple identifiers, including EVENT_MESSAGE_ID_COLUMN.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant GraphQL
    participant Database

    Client->>GraphQL: Subscribe to entityUpdated
    GraphQL->>Database: Listen for changes on Entity
    Database-->>GraphQL: Notify on entity update
    GraphQL-->>Client: Send updated entity data
Loading

Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between da3a0a6 and d77f452.

Files selected for processing (1)
  • crates/torii/core/src/sql_test.rs (7 hunks)
Additional comments not posted (3)
crates/torii/core/src/sql_test.rs (3)

97-103: Great use of TransactionWaiter!

Replacing tokio::time::sleep with TransactionWaiter ensures that the test waits for transaction confirmation, which enhances reliability and aligns with best practices in asynchronous programming.


240-246: Consistent use of TransactionWaiter!

The consistent application of TransactionWaiter across test functions enhances test reliability and synchronization.

Consider verifying that all transaction-related tests have been updated similarly.

Verification successful

Ohayo, sensei! Consistent use of TransactionWaiter verified.

The search results confirm that TransactionWaiter is consistently used across multiple transaction-related tests, enhancing reliability and synchronization as intended.

  • Verified occurrences in files like sql_test.rs, messaging.rs, and others.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all transaction-related tests use `TransactionWaiter`.

# Test: Search for usage of `TransactionWaiter`. Expect: Occurrences in all transaction-related tests.
rg --type rust 'TransactionWaiter::new'

Length of output: 4531


352-358: Solid implementation with TransactionWaiter!

The use of TransactionWaiter maintains consistency and reliability across the test suite.

Ensure that all relevant tests are updated to use TransactionWaiter.

Verification successful

Ohayo, sensei! Consistent Use of TransactionWaiter Confirmed.

The TransactionWaiter is consistently used across various test files, ensuring reliability and uniformity in transaction handling throughout the test suite.

  • Files such as sql_test.rs, graphql/src/tests/mod.rs, and others in the sozo and dojo-world directories utilize TransactionWaiter.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all relevant tests use `TransactionWaiter`.

# Test: Search for usage of `TransactionWaiter`. Expect: Consistent usage across all relevant tests.
rg --type rust 'TransactionWaiter::new'

Length of output: 4531


Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Jul 29, 2024

Codecov Report

Attention: Patch coverage is 22.91667% with 37 lines in your changes missing coverage. Please review.

Project coverage is 69.60%. Comparing base (aca93b4) to head (da3a0a6).
Report is 3 commits behind head on main.

Files Patch % Lines
crates/torii/graphql/src/object/event_message.rs 0.00% 23 Missing ⚠️
crates/torii/graphql/src/object/model_data.rs 33.33% 12 Missing ⚠️
crates/torii/graphql/src/object/entity.rs 80.00% 1 Missing ⚠️
crates/torii/graphql/src/query/mod.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2227      +/-   ##
==========================================
- Coverage   69.60%   69.60%   -0.01%     
==========================================
  Files         352      352              
  Lines       46019    46032      +13     
==========================================
+ Hits        32030    32039       +9     
- Misses      13989    13993       +4     

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Comment on lines 268 to 296
fn event_message_field() -> Field {
Field::new("eventMessage", TypeRef::named("EventMessage"), |ctx| {
FieldFuture::new(async move {
match ctx.parent_value.try_to_value()? {
Value::Object(indexmap) => {
let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?;
let entity_id = utils::extract::<String>(indexmap, INTERNAL_ENTITY_ID_KEY)?;
let data =
fetch_single_row(&mut conn, EVENT_MESSAGE_TABLE, ID_COLUMN, &entity_id)
.await?;
let event_message = value_mapping_from_row(&data, &ENTITY_TYPE_MAPPING, false)?;

Ok(Some(Value::Object(event_message)))
}
_ => Err("incorrect value, requires Value::Object".into()),
}
})
})
}
Copy link

Choose a reason for hiding this comment

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

Ohayo, sensei! The new event_message_field function looks solid!

The function is well-structured and follows best practices for asynchronous database access and error handling.

Consider adding more detailed error messages to improve debugging.

-  _ => Err("incorrect value, requires Value::Object".into()),
+  _ => Err("Incorrect value type encountered. Expected Value::Object.".into()),

Ensure that this function is covered by unit tests.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn event_message_field() -> Field {
Field::new("eventMessage", TypeRef::named("EventMessage"), |ctx| {
FieldFuture::new(async move {
match ctx.parent_value.try_to_value()? {
Value::Object(indexmap) => {
let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?;
let entity_id = utils::extract::<String>(indexmap, INTERNAL_ENTITY_ID_KEY)?;
let data =
fetch_single_row(&mut conn, EVENT_MESSAGE_TABLE, ID_COLUMN, &entity_id)
.await?;
let event_message = value_mapping_from_row(&data, &ENTITY_TYPE_MAPPING, false)?;
Ok(Some(Value::Object(event_message)))
}
_ => Err("incorrect value, requires Value::Object".into()),
}
})
})
}
fn event_message_field() -> Field {
Field::new("eventMessage", TypeRef::named("EventMessage"), |ctx| {
FieldFuture::new(async move {
match ctx.parent_value.try_to_value()? {
Value::Object(indexmap) => {
let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?;
let entity_id = utils::extract::<String>(indexmap, INTERNAL_ENTITY_ID_KEY)?;
let data =
fetch_single_row(&mut conn, EVENT_MESSAGE_TABLE, ID_COLUMN, &entity_id)
.await?;
let event_message = value_mapping_from_row(&data, &ENTITY_TYPE_MAPPING, false)?;
Ok(Some(Value::Object(event_message)))
}
_ => Err("Incorrect value type encountered. Expected Value::Object.".into()),
}
})
})
}

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Comment on lines 273 to 296
fn event_message_field() -> Field {
Field::new("eventMessage", TypeRef::named(EVENT_MESSAGE_TYPE_NAME), |ctx| {
FieldFuture::new(async move {
match ctx.parent_value.try_to_value()? {
Value::Object(indexmap) => {
let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?;
let entity_id = utils::extract::<String>(indexmap, INTERNAL_ENTITY_ID_KEY)?;
let data =
fetch_single_row(&mut conn, EVENT_MESSAGE_TABLE, ID_COLUMN, &entity_id)
.await?;
let event_message = value_mapping_from_row(
&data,
EVENT_MESSAGE_ID_COLUMN,
&ENTITY_TYPE_MAPPING,
false,
)?;

Ok(Some(Value::Object(event_message)))
}
_ => Err("incorrect value, requires Value::Object".into()),
}
})
})
}
Copy link

Choose a reason for hiding this comment

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

Ohayo, sensei! The new event_message_field function looks solid!

The function is well-structured and follows best practices for asynchronous database access and error handling.

Consider adding more detailed error messages to improve debugging.

-  _ => Err("incorrect value, requires Value::Object".into()),
+  _ => Err("Incorrect value type encountered. Expected Value::Object.".into()),

Ensure that this function is covered by unit tests.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn event_message_field() -> Field {
Field::new("eventMessage", TypeRef::named(EVENT_MESSAGE_TYPE_NAME), |ctx| {
FieldFuture::new(async move {
match ctx.parent_value.try_to_value()? {
Value::Object(indexmap) => {
let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?;
let entity_id = utils::extract::<String>(indexmap, INTERNAL_ENTITY_ID_KEY)?;
let data =
fetch_single_row(&mut conn, EVENT_MESSAGE_TABLE, ID_COLUMN, &entity_id)
.await?;
let event_message = value_mapping_from_row(
&data,
EVENT_MESSAGE_ID_COLUMN,
&ENTITY_TYPE_MAPPING,
false,
)?;
Ok(Some(Value::Object(event_message)))
}
_ => Err("incorrect value, requires Value::Object".into()),
}
})
})
}
fn event_message_field() -> Field {
Field::new("eventMessage", TypeRef::named(EVENT_MESSAGE_TYPE_NAME), |ctx| {
FieldFuture::new(async move {
match ctx.parent_value.try_to_value()? {
Value::Object(indexmap) => {
let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?;
let entity_id = utils::extract::<String>(indexmap, INTERNAL_ENTITY_ID_KEY)?;
let data =
fetch_single_row(&mut conn, EVENT_MESSAGE_TABLE, ID_COLUMN, &entity_id)
.await?;
let event_message = value_mapping_from_row(
&data,
EVENT_MESSAGE_ID_COLUMN,
&ENTITY_TYPE_MAPPING,
false,
)?;
Ok(Some(Value::Object(event_message)))
}
_ => Err("Incorrect value type encountered. Expected Value::Object.".into()),
}
})
})
}

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Comment on lines +268 to +286
fn event_message_field() -> Field {
Field::new("eventMessage", TypeRef::named(EVENT_MESSAGE_TYPE_NAME), |ctx| {
FieldFuture::new(async move {
match ctx.parent_value.try_to_value()? {
Value::Object(indexmap) => {
let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?;
let entity_id = utils::extract::<String>(indexmap, INTERNAL_ENTITY_ID_KEY)?;
let data =
fetch_single_row(&mut conn, EVENT_MESSAGE_TABLE, ID_COLUMN, &entity_id)
.await?;
let event_message = value_mapping_from_row(&data, &ENTITY_TYPE_MAPPING, false)?;

Ok(Some(Value::Object(event_message)))
}
_ => Err("incorrect value, requires Value::Object".into()),
}
})
})
}
Copy link

Choose a reason for hiding this comment

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

Ohayo, sensei! The new event_message_field function looks solid!

The function is well-structured and follows best practices for asynchronous database access and error handling.

Consider adding more detailed error messages to improve debugging.

-  _ => Err("incorrect value, requires Value::Object".into()),
+  _ => Err("Incorrect value type encountered. Expected Value::Object.".into()),

Ensure that this function is covered by unit tests.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn event_message_field() -> Field {
Field::new("eventMessage", TypeRef::named(EVENT_MESSAGE_TYPE_NAME), |ctx| {
FieldFuture::new(async move {
match ctx.parent_value.try_to_value()? {
Value::Object(indexmap) => {
let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?;
let entity_id = utils::extract::<String>(indexmap, INTERNAL_ENTITY_ID_KEY)?;
let data =
fetch_single_row(&mut conn, EVENT_MESSAGE_TABLE, ID_COLUMN, &entity_id)
.await?;
let event_message = value_mapping_from_row(&data, &ENTITY_TYPE_MAPPING, false)?;
Ok(Some(Value::Object(event_message)))
}
_ => Err("incorrect value, requires Value::Object".into()),
}
})
})
}
fn event_message_field() -> Field {
Field::new("eventMessage", TypeRef::named(EVENT_MESSAGE_TYPE_NAME), |ctx| {
FieldFuture::new(async move {
match ctx.parent_value.try_to_value()? {
Value::Object(indexmap) => {
let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?;
let entity_id = utils::extract::<String>(indexmap, INTERNAL_ENTITY_ID_KEY)?;
let data =
fetch_single_row(&mut conn, EVENT_MESSAGE_TABLE, ID_COLUMN, &entity_id)
.await?;
let event_message = value_mapping_from_row(&data, &ENTITY_TYPE_MAPPING, false)?;
Ok(Some(Value::Object(event_message)))
}
_ => Err("Incorrect value type encountered. Expected Value::Object.".into()),
}
})
})
}

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

use sqlx::{Pool, Sqlite};
use tokio_stream::StreamExt;
use torii_core::simple_broker::SimpleBroker;
use torii_core::types::EventMessage;

use super::entity::model_data_recursive_query;
Copy link

Choose a reason for hiding this comment

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

Ohayo, sensei! Consider removing unused imports.

The model_data_recursive_query function is removed, but its import still exists. Removing unused imports helps maintain clean and efficient code.

- use super::entity::model_data_recursive_query;
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
use super::entity::model_data_recursive_query;

for (id, name) in model_ids {
// the model id is used as the id for the model members
for (id, namespace, name) in model_ids {
// the model id in the model mmeebrs table is the hashed model name (id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// the model id in the model mmeebrs table is the hashed model name (id)
// the model id in the model members table is the hashed model name (id)

@glihm glihm merged commit ccbccbf into dojoengine:main Aug 13, 2024
11 checks passed
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