Seanaye/feat/gql#4236
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a new 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seanaye
force-pushed
the
seanaye/feat/gql
branch
from
5c3b3e4 to
a1bbe05
Compare
seanaye
force-pushed
the
seanaye/feat/soup-threads-4
branch
from
8776548 to
722c409
Compare
seanaye
force-pushed
the
seanaye/feat/gql
branch
from
a1bbe05 to
c8f33c3
Compare
seanaye
force-pushed
the
seanaye/feat/gql
branch
from
c8f33c3 to
72b3fe2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/cloud-storage/graphql_soup/src/lib.rs`:
- Around line 1-5: The lib.rs file is missing the required
`#![deny(missing_docs)]` directive that enforces documentation on all public
items. Add the `#![deny(missing_docs)]` attribute at the beginning of the lib.rs
file, immediately after the existing module-level documentation comment (the //!
lines) and before any other code. This ensures all public items in the crate
must have documentation.
- Around line 86-98: The entity type parsing in the map closure is failing
because model_entity EntityType representations don't align with
models_properties::EntityType. When parsing entity types from
self.0.entity_type().to_string() using
models_properties::EntityType::from_str(), three types cause runtime failures:
EmailThread maps to "email_thread" but models_properties expects "thread",
ChannelThread and ForeignEntity have no equivalent in models_properties. Either
extend models_properties::EntityType to include mappings for these missing
entity types (mapping "email_thread" to "thread" and adding support for
"channel_thread" and "foreign_entity"), or filter out these entity types before
they reach the entity_refs mapping logic in the properties method.
In `@rust/cloud-storage/notification/src/outbound/repository.rs`:
- Around line 1186-1198: The loop iterating through entity_refs at the match
operation is processing duplicate entity reference values, causing the same
notification to be added multiple times to the same entity_ref bucket in the
result map. Deduplicate the entity_refs collection before the for loop begins
(before the iteration over &entity_refs) to ensure each unique entity_ref is
only processed once. This prevents duplicate notifications from being inserted
into the result map entry for a given entity_ref key, while maintaining the
correct matching logic in the notification_ref_matches_row function call and the
subsequent push operation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5f142fbe-aa33-4453-b991-85b21a64892c
⛔ Files ignored due to path filters (1)
rust/cloud-storage/Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (27)
plan.mdrust/cloud-storage/Cargo.tomlrust/cloud-storage/document_storage_service/Cargo.tomlrust/cloud-storage/document_storage_service/src/api/context.rsrust/cloud-storage/document_storage_service/src/api/graphql_soup.rsrust/cloud-storage/document_storage_service/src/api/mod.rsrust/cloud-storage/document_storage_service/src/main.rsrust/cloud-storage/document_storage_service/src/service/mod.rsrust/cloud-storage/graphql_soup/Cargo.tomlrust/cloud-storage/graphql_soup/src/lib.rsrust/cloud-storage/notification/src/domain/models/request.rsrust/cloud-storage/notification/src/domain/ports.rsrust/cloud-storage/notification/src/domain/service/device/test.rsrust/cloud-storage/notification/src/domain/service/ingress.rsrust/cloud-storage/notification/src/domain/service/push_notification_event/test.rsrust/cloud-storage/notification/src/domain/service/test.rsrust/cloud-storage/notification/src/domain/service/voip/test.rsrust/cloud-storage/notification/src/outbound/repository.rsrust/cloud-storage/notification_sandbox/src/adapters/sandbox_repository.rsrust/cloud-storage/properties/Cargo.tomlrust/cloud-storage/properties/src/domain/model.rsrust/cloud-storage/properties/src/domain/ports.rsrust/cloud-storage/properties/src/domain/service.rsrust/cloud-storage/properties/src/domain/service_impl/mod.rsrust/cloud-storage/properties/src/lib.rsrust/cloud-storage/properties/src/outbound/properties_pg_repo.rsrust/cloud-storage/soup/src/inbound/axum_router.rs
| //! GraphQL inbound adapter for Soup. | ||
| //! | ||
| //! This crate is intentionally additive: it maps GraphQL requests onto the | ||
| //! existing `soup` domain service without changing the existing REST API. | ||
|
|
There was a problem hiding this comment.
Add #![deny(missing_docs)] directive to enforce documentation on all public items.
Per coding guidelines, new crates must include #![deny(missing_docs)] in lib.rs to enforce documentation on all public items.
📝 Proposed fix
//! GraphQL inbound adapter for Soup.
//!
//! This crate is intentionally additive: it maps GraphQL requests onto the
//! existing `soup` domain service without changing the existing REST API.
+#![deny(missing_docs)]
use async_graphql::{📝 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.
| //! GraphQL inbound adapter for Soup. | |
| //! | |
| //! This crate is intentionally additive: it maps GraphQL requests onto the | |
| //! existing `soup` domain service without changing the existing REST API. | |
| //! GraphQL inbound adapter for Soup. | |
| //! | |
| //! This crate is intentionally additive: it maps GraphQL requests onto the | |
| //! existing `soup` domain service without changing the existing REST API. | |
| #![deny(missing_docs)] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/graphql_soup/src/lib.rs` around lines 1 - 5, The lib.rs
file is missing the required `#![deny(missing_docs)]` directive that enforces
documentation on all public items. Add the `#![deny(missing_docs)]` attribute at
the beginning of the lib.rs file, immediately after the existing module-level
documentation comment (the //! lines) and before any other code. This ensures
all public items in the crate must have documentation.
Source: Coding guidelines
| let entity_refs = keys | ||
| .iter() | ||
| .map(|key| { | ||
| let entity_type = models_properties::EntityType::from_str(&key.entity_type) | ||
| .map_err(|err| { | ||
| rootcause::report!( | ||
| "invalid entity type {} for property edge: {err}", | ||
| key.entity_type | ||
| ) | ||
| })?; | ||
| Ok(EntityReference::new(key.entity_id.clone(), entity_type)) | ||
| }) | ||
| .collect::<Result<Vec<_>, rootcause::Report>>()?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify EntityType string conversion compatibility
# Check model_entity::EntityType Display/ToString impl
echo "=== Checking model_entity::EntityType ==="
rg -nP --type=rust -A10 -B2 'impl.*Display.*for.*EntityType' rust/cloud-storage/model-entity/
# Check models_properties::EntityType FromStr impl
echo "=== Checking models_properties::EntityType FromStr ==="
rg -nP --type=rust -A10 -B2 'impl.*FromStr.*for.*EntityType' rust/cloud-storage/models_properties/
# Check if they are the same type (re-export)
echo "=== Checking if EntityType is re-exported ==="
rg -nP --type=rust 'pub use.*EntityType' rust/cloud-storage/models_properties/ rust/cloud-storage/model-entity/Repository: macro-inc/macro
Length of output: 1787
🏁 Script executed:
# Find model_entity::EntityType definition
fd --type=f '\.rs$' rust/cloud-storage/model-entity | xargs grep -l 'enum EntityType\|struct EntityType' | head -5
# Check the actual definition
find rust/cloud-storage/model-entity -name '*.rs' -exec grep -l 'EntityType' {} \;Repository: macro-inc/macro
Length of output: 190
🏁 Script executed:
# Examine graphql_soup at line 577 mentioned in the scratchpad
cat -n rust/cloud-storage/graphql_soup/src/lib.rs | sed -n '570,590p'Repository: macro-inc/macro
Length of output: 892
🏁 Script executed:
# Check if model_entity is re-exported or if there are multiple EntityType types
rg 'enum EntityType|struct EntityType' rust/cloud-storage/model-entity/ -A5Repository: macro-inc/macro
Length of output: 487
🏁 Script executed:
# Get the full EntityType enum definition from model-entity
cat -n rust/cloud-storage/model-entity/src/lib.rs | grep -A30 'pub enum EntityType'Repository: macro-inc/macro
Length of output: 1174
🏁 Script executed:
# Check if model-entity::EntityType has a Display impl or derives Debug
rg 'impl.*Display.*EntityType|impl.*Debug|#\[derive' rust/cloud-storage/model-entity/src/lib.rs -B5 -A15 | head -100Repository: macro-inc/macro
Length of output: 1069
🏁 Script executed:
# Check the exact Display impl behavior with EnumString and serde attributes
cat -n rust/cloud-storage/model-entity/src/lib.rs | sed -n '20,65p'Repository: macro-inc/macro
Length of output: 1530
🏁 Script executed:
# Verify which entity types are actually used as Document entities in graphql_soup
rg -B5 -A5 'GraphqlSoupDocument' rust/cloud-storage/graphql_soup/src/lib.rs | head -40Repository: macro-inc/macro
Length of output: 1256
🏁 Script executed:
# Check what entity types models_properties actually supports
cat -n rust/cloud-storage/models_properties/src/shared/entity_type.rsRepository: macro-inc/macro
Length of output: 2761
🏁 Script executed:
# Verify what entity types are actually used in SoupPropertyEdgeReader
rg 'EntityPropertiesKey' rust/cloud-storage/graphql_soup/src/lib.rs -B10 -A5Repository: macro-inc/macro
Length of output: 2931
🏁 Script executed:
# Check what entity types can reach the SoupPropertyEdgeReader batch loader
rg 'SoupPropertyEdgeReader|batch.*SoupProperty' rust/cloud-storage/ -r 'rust/cloud-storage/graphql_soup' -A5 -B5Repository: macro-inc/macro
Length of output: 4260
Fix incompatible EntityType mappings between model_entity and models_properties.
The properties method on GraphqlSoupDocument (line 577) calls self.0.entity_type().to_string() which returns a model_entity representation, then attempts to parse it with models_properties::EntityType::from_str() at line 89. However, three entity types that appear in GraphqlSoupEntity will fail:
EmailThreadserializes to"email_thread"but models_properties only has"thread"ChannelThreadserializes to"channel_thread"but is not in models_properties at allForeignEntityserializes to"foreign_entity"but is not in models_properties at all
This causes runtime parse failures with NoConversion errors when loading properties for these entity types. Either map these missing types in models_properties::EntityType or prevent these entity types from reaching the property edge loader.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/graphql_soup/src/lib.rs` around lines 86 - 98, The entity
type parsing in the map closure is failing because model_entity EntityType
representations don't align with models_properties::EntityType. When parsing
entity types from self.0.entity_type().to_string() using
models_properties::EntityType::from_str(), three types cause runtime failures:
EmailThread maps to "email_thread" but models_properties expects "thread",
ChannelThread and ForeignEntity have no equivalent in models_properties. Either
extend models_properties::EntityType to include mappings for these missing
entity types (mapping "email_thread" to "thread" and adding support for
"channel_thread" and "foreign_entity"), or filter out these entity types before
they reach the entity_refs mapping logic in the properties method.
| for entity_ref in &entity_refs { | ||
| if notification_ref_matches_row( | ||
| entity_ref, | ||
| &event_item_id, | ||
| &event_item_type, | ||
| ¬ification_event_type, | ||
| ¬ification_metadata, | ||
| ) { | ||
| result | ||
| .entry(entity_ref.clone()) | ||
| .or_default() | ||
| .push(notification.clone()); | ||
| } |
There was a problem hiding this comment.
Deduplicate entity refs before distributing matched notifications
At Line 1186, iterating the original entity_refs can duplicate entries in a bucket when callers pass duplicate refs. The map key is already unique, so values should not be multiplied by duplicate input keys.
Proposed fix
let mut result = entity_refs
.iter()
.cloned()
.map(|entity_ref| (entity_ref, Vec::new()))
.collect::<HashMap<_, _>>();
+ let match_entity_refs: Vec<NotificationEntityRef> = result.keys().cloned().collect();
if entity_refs.is_empty() {
return Ok(result);
}
@@
- for entity_ref in &entity_refs {
+ for entity_ref in &match_entity_refs {
if notification_ref_matches_row(
entity_ref,
&event_item_id,
&event_item_type,
¬ification_event_type,
¬ification_metadata,
) {
result
.entry(entity_ref.clone())
.or_default()
.push(notification.clone());
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/notification/src/outbound/repository.rs` around lines 1186
- 1198, The loop iterating through entity_refs at the match operation is
processing duplicate entity reference values, causing the same notification to
be added multiple times to the same entity_ref bucket in the result map.
Deduplicate the entity_refs collection before the for loop begins (before the
iteration over &entity_refs) to ensure each unique entity_ref is only processed
once. This prevents duplicate notifications from being inserted into the result
map entry for a given entity_ref key, while maintaining the correct matching
logic in the notification_ref_matches_row function call and the subsequent push
operation.
1ca571d to
1a348e2
Compare
seanaye
force-pushed
the
seanaye/feat/gql
branch
from
8ae1bfe to
85b5cc7
Compare
1 participant
No description provided.