feat: block instances and actors #67
Conversation
WalkthroughThe pull request introduces functionality for blocking and unblocking URLs within the Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (23)
crates/apub/src/utils/mod.rs (1)
1-3: LGTM! Consider adding a brief doc comment.The changes look good and align with the PR objectives. The new module and function export are correctly implemented.
Consider adding a brief doc comment above the
pub usestatement to explain the purpose of theverify_blockedfunction. This would improve the module's documentation and make it easier for other developers to understand its functionality. For example:/// Checks if a given URL (instance or actor) is blocked. pub use verify_blocked::verify_blocked;crates/api_admin/src/entities/block_url_or_acct.rs (3)
5-8: LGTM: Well-structured query type for blocking URLs.The
BlockUrlQuerystruct is appropriately designed for handling block requests. The use ofDeserializeandIntoParamsderive macros facilitates easy request parsing and API documentation.Consider adding a doc comment to explain the purpose of this struct, e.g.:
/// Represents a request to block a URL or account. #[derive(Deserialize, IntoParams)] pub struct BlockUrlQuery { pub url: Url, }
10-14: LGTM: Well-structured result type for block operations.The
BlockUrlResultstruct is appropriately designed for returning the results of block operations. The use ofSerializeandToSchemaderive macros facilitates easy response serialization and API documentation.Consider adding a doc comment to explain the purpose of this struct and its fields, e.g.:
/// Represents the result of a URL or account blocking operation. #[derive(Serialize, ToSchema)] pub struct BlockUrlResult { /// The URL or account that was blocked or unblocked. pub url: Url, /// A message describing the result of the operation. pub message: String, }
1-14: Great foundation for implementing URL and account blocking functionality.This file provides well-structured types for handling block requests and responses, aligning perfectly with the PR objectives and the API endpoints described in the linked issue. The use of appropriate derive macros and the
Urltype ensures robust handling of inputs and outputs.Consider adding a third struct to represent the account string format (e.g.,
foo@instance.tld) mentioned in the PR objectives. This could be used for the/api/v0/admin/block_acctendpoint. For example:#[derive(Deserialize, IntoParams)] pub struct BlockAcctQuery { pub acct: String, }This would provide a clear distinction between URL-based and account-based blocking requests.
crates/db_schema/src/blocked_url.rs (2)
5-11: LGTM: Model struct is well-defined. Consider adding documentation.The
Modelstruct is correctly defined with appropriate fields and attributes for theblocked_urltable. Theidfield as a non-auto-incrementing primary key is suitable for storing URLs, and theis_instanceboolean allows distinguishing between blocked instances and individual actors.Consider adding documentation comments to the struct and its fields to improve code readability:
#[derive(Clone, Debug, PartialEq, DeriveEntityModel, Eq)] #[sea_orm(table_name = "blocked_url")] +/// Represents a blocked URL in the database pub struct Model { #[sea_orm(primary_key, auto_increment = false)] + /// The URL of the blocked instance or actor pub id: String, + /// Indicates whether this is a blocked instance (true) or an individual actor (false) pub is_instance: bool, }
13-16: LGTM: Relation enum and ActiveModelBehavior implementation are correct.The empty
Relationenum and the implementation ofActiveModelBehaviorforActiveModelare appropriate for this entity.While the current implementation is correct, consider the following suggestions for future improvements:
- If relationships with other entities are added in the future, update the
Relationenum accordingly.- If custom behavior is needed for creating or updating
blocked_urlentries, consider implementing custom methods in theActiveModelBehaviortrait.Example of potential future custom behavior:
impl ActiveModelBehavior for ActiveModel { fn before_save(self, insert: bool) -> Result<Self, DbErr> { // Validate URL format // Normalize URL (e.g., remove trailing slashes) // Check for duplicates Ok(self) } }crates/apub/src/lib.rs (1)
6-6: Newutilsmodule added: Consider documentation and visibility.The addition of the
utilsmodule is a good way to organize utility functions related to theapubcrate. However, there are a few points to consider:
The module is currently private (
mod utils;). Depending on its intended use, you might want to make it public (pub mod utils;) if other crates or modules need to access its contents.It would be helpful to add a brief comment explaining the purpose of the
utilsmodule and what kind of utilities it contains. This improves code readability and maintainability.Ensure that the
utilsmodule is properly documented internally, especially if it contains public functions or structures.Consider adding a brief comment above the module declaration, like this:
/// Utility functions and helpers for ActivityPub operations mod utils;Also, if the module is intended to be used outside this crate, change it to:
/// Utility functions and helpers for ActivityPub operations pub mod utils;crates/db_migration/src/m20240926_000001_blocked_url.rs (2)
6-21: LGTM: Migrationupmethod is well-implemented.The
upmethod correctly creates theBlockedUrltable with the required columns. The use ofif_not_exists()ensures idempotency, which is a good practice for migrations.Consider adding a comment explaining the purpose of the
IsInstancecolumn, similar to the comment for theIdcolumn in theBlockedUrlenum. This would improve code readability.
32-39: LGTM:BlockedUrlenum is well-defined.The enum correctly represents the table and its columns, following the
sea_orm_migrationpattern. The comments provide useful context for the columns' purposes.Consider moving the comment for
IsInstanceto the line directly above the variant for consistency with theIdcomment style:// is instance (if false, then this is actor) IsInstance,crates/db_migration/src/m20240131_000004_activity.rs (2)
38-38: Approve comment change with a minor suggestionThe updated comment accurately reflects the new purpose of the
Idfield, which is now intended to store a URL instead of a UUID. This change is consistent with the data type modification we observed earlier.For even better clarity, consider expanding the comment slightly:
- // Activity URL + // Activity URL (used as unique identifier)This addition explicitly states that the URL serves as the unique identifier for the activity, which might be helpful for future developers working with this schema.
Line range hint
1-51: Summary of changes and potential system-wide impactThe changes in this migration script successfully implement the use of URLs as activity identifiers, which aligns with the PR objectives for blocking instances and actors. However, these modifications have potential system-wide implications that should be carefully considered:
- Data migration: Existing data in the
Activitytable may need to be migrated from UUID to URL format.- Performance: The switch from UUID to string-based primary keys might affect indexing and query performance.
- Code dependencies: Other parts of the system that expect
Activity::Idto be a UUID may need to be updated.- Database constraints: Consider adding a unique constraint or index on the
Idcolumn to ensure URL uniqueness.Before merging this PR, I recommend:
- Creating a data migration plan for existing activities (if any).
- Reviewing and updating all code that interacts with
Activity::Id.- Considering adding a database index on the
Idcolumn for performance.- Updating any API documentation to reflect the new
Idformat.- Ensuring that input validation is in place to guarantee that only valid URLs are stored in the
Idfield.These steps will help ensure a smooth transition to the new activity identifier format and maintain system integrity.
crates/apub/src/activities/like_or_announce/undo_like_or_announce.rs (1)
Line range hint
14-42: Overall implementation of block verification looks good.The changes successfully implement block verification for the
UndoLikeOrAnnounceactivity, which aligns with the PR objectives of implementing instance and actor blocking. The code structure and implementation appear correct.Next steps:
- Address the TODO comment in the
verifymethod.- Ensure that similar block verification is implemented consistently across other relevant activities in the codebase.
- Consider adding tests to verify the blocking functionality for this activity.
To maintain consistency and reduce code duplication, consider creating a trait or a macro that can be easily applied to all activities requiring block verification. This would ensure that the blocking logic is uniformly implemented across the codebase.
crates/apub/src/activities/following/undo_follow.rs (2)
47-51: LGTM: Blocking verification implemented correctly.The
verifymethod has been updated to use theverify_blockedfunction, which aligns with the PR objectives of implementing instance and actor blocking. This change enhances the verification process by checking if the actor is blocked before proceeding.However, there's a TODO comment that needs to be addressed:
Would you like me to help remove the TODO comment or suggest additional verification steps that might be needed here?
Line range hint
1-70: Summary: Changes align well with PR objectives.The modifications to this file contribute to the implementation of the blocking functionality as outlined in the PR objectives. The
verifymethod now includes a check to determine if an actor is blocked, which is a key component of the blocking mechanism. This change ensures that blocked instances or actors are prevented from performing certain actions, in this case, undoing a follow.To further improve the implementation:
- Consider adding more comprehensive verification steps if needed (as hinted by the TODO comment).
- Ensure that this blocking check is consistently applied across all relevant activity types.
crates/apub/src/activities/following/follow.rs (1)
62-65: LGTM: Blocking verification implemented correctly.The changes to the
verifymethod successfully implement the blocking check for Follow activities:
- The
dataparameter addition allows access to necessary application data.- The
verify_blockedfunction is correctly called and its error is properly propagated.These changes align well with the PR objectives of implementing blocking functionality.
Consider addressing or removing the TODO comment if it's no longer relevant.
crates/apub/src/activities/like_or_announce/like_or_announce.rs (2)
52-55: LGTM: Blocking verification added.The implementation of
verify_blockedin theverifymethod aligns with the PR objectives. The function is called with appropriate parameters and the result is properly handled.However, there's a TODO comment that should be addressed:
Consider removing or replacing the TODO comment with a brief explanation of what the
verifymethod does, especially in relation to the new blocking functionality.
25-25: Summary: Blocking feature successfully implemented.The changes in this file effectively implement the blocking functionality as per the PR objectives. The
verifymethod now checks if a URL is blocked before proceeding, which enhances the security and control of the system. The implementation is concise and follows good practices.To further improve the code:
- Consider adding a brief comment explaining the purpose of the
verifymethod and how it integrates with the blocking feature.- Ensure that comprehensive tests are in place to verify the blocking functionality in various scenarios.
Also applies to: 52-55
crates/apub/src/activities/create_or_update/note.rs (2)
101-101: LGTM: New verification step for blocked users.The addition of
verify_blocked(&self.id, data).await?implements the blocking functionality as specified in the PR objectives. This check ensures that blocked users are prevented from interacting with the system.Consider adding more specific error handling for the case when a user is blocked. This could provide clearer feedback and potentially log the blocked interaction attempt.
Here's a suggested implementation:
match verify_blocked(&self.id, data).await { Ok(_) => Ok(()), Err(BlockedError::UserBlocked) => { log::warn!("Blocked user attempted interaction: {:?}", self.id); Err(AppError::UserBlocked) }, Err(e) => Err(e.into()), }This assumes the existence of a
BlockedErrorenum and anAppError::UserBlockedvariant. If these don't exist, you may need to add them to your error handling structure.
Line range hint
97-97: Address TODO comments inverifyandreceivemethods.There are two TODO comments in the file that need attention:
- In the
verifymethod (line 97)- In the
receivemethod for theUpdateTypecase (line 109)These comments indicate incomplete implementation of some functionality. It's important to address these to ensure full feature implementation and to reduce technical debt.
Would you like me to help create GitHub issues to track these TODOs? This can help ensure they're not overlooked in future development cycles.
Also applies to: 109-109
crates/apub/src/utils/verify_blocked.rs (4)
19-23: Use appropriate HTTP status code for blocked instances.Returning
StatusCode::BAD_REQUESTwhen a URL is blocked might not accurately reflect the nature of the error. Consider usingStatusCode::FORBIDDEN(403) to indicate that access to the resource is forbidden.Apply this diff to change the status code:
Some(StatusCode::BAD_REQUEST), +Some(StatusCode::FORBIDDEN),
20-20: Avoid exposing internal information in error messages.The error message
blocked instance: {:?}", url.host_str()reveals the host of the blocked URL, which may not be necessary. For security and user experience, consider providing a more generic message.Apply this diff to modify the error message:
-format!("blocked instance: {:?}", url.host_str()), +String::from("Access to this instance is forbidden."),
31-31: Avoid exposing the blocked URL in error messages.Similarly, the error message
blocked actor: {}", urlexposes the full URL of the blocked actor. It's better to return a generic message to prevent disclosing sensitive information.Apply this diff to modify the error message:
-format!("blocked actor: {}", url), +String::from("Access to this actor is forbidden."),
8-38: Optimize database queries to improve performance.Fetching all blocked URLs and filtering them in-memory can be inefficient, especially as the list grows. Consider performing the filtering directly in the database query to reduce memory usage and improve performance.
Refactor the code to filter blocked instances and actors at the database level:
use hatsu_db_schema::blocked_url::Entity as BlockedUrlEntity; use hatsu_db_schema::blocked_url::Column; use sea_orm::QueryFilter; // Check if the URL's origin is a blocked instance let origin = url.origin().ascii_serialization(); let is_blocked_instance = BlockedUrlEntity::find() .filter(Column::IsInstance.eq(true)) .filter(Column::Id.eq(origin.clone())) .one(&data.conn) .await? .is_some(); if is_blocked_instance { return Err(AppError::new( String::from("Access to this instance is forbidden."), None, Some(StatusCode::FORBIDDEN), )); } // Check if the exact URL is a blocked actor let is_blocked_actor = BlockedUrlEntity::find() .filter(Column::IsInstance.eq(false)) .filter(Column::Id.eq(url.as_str())) .one(&data.conn) .await? .is_some(); if is_blocked_actor { return Err(AppError::new( String::from("Access to this actor is forbidden."), None, Some(StatusCode::FORBIDDEN), )); } Ok(())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockflake.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
- crates/api_admin/Cargo.toml (1 hunks)
- crates/api_admin/src/entities/block_url_or_acct.rs (1 hunks)
- crates/api_admin/src/entities/mod.rs (1 hunks)
- crates/api_admin/src/routes/block_url.rs (1 hunks)
- crates/api_admin/src/routes/mod.rs (3 hunks)
- crates/api_admin/src/routes/unblock_url.rs (1 hunks)
- crates/apub/src/activities/create_or_update/note.rs (3 hunks)
- crates/apub/src/activities/db_activity_impl.rs (1 hunks)
- crates/apub/src/activities/following/accept_follow.rs (2 hunks)
- crates/apub/src/activities/following/follow.rs (2 hunks)
- crates/apub/src/activities/following/undo_follow.rs (2 hunks)
- crates/apub/src/activities/like_or_announce/like_or_announce.rs (2 hunks)
- crates/apub/src/activities/like_or_announce/undo_like_or_announce.rs (2 hunks)
- crates/apub/src/lib.rs (1 hunks)
- crates/apub/src/utils/mod.rs (1 hunks)
- crates/apub/src/utils/verify_blocked.rs (1 hunks)
- crates/db_migration/src/lib.rs (2 hunks)
- crates/db_migration/src/m20240131_000004_activity.rs (2 hunks)
- crates/db_migration/src/m20240926_000001_blocked_url.rs (1 hunks)
- crates/db_schema/src/activity.rs (2 hunks)
- crates/db_schema/src/blocked_url.rs (1 hunks)
- crates/db_schema/src/lib.rs (1 hunks)
- crates/db_schema/src/post.rs (2 hunks)
- crates/db_schema/src/prelude.rs (1 hunks)
- crates/db_schema/src/received_announce.rs (1 hunks)
- crates/db_schema/src/received_follow.rs (2 hunks)
- crates/db_schema/src/received_like.rs (1 hunks)
- crates/db_schema/src/user.rs (3 hunks)
- crates/db_schema/src/user_feed_item.rs (1 hunks)
✅ Files skipped from review due to trivial changes (3)
- crates/db_schema/src/received_announce.rs
- crates/db_schema/src/received_like.rs
- crates/db_schema/src/user_feed_item.rs
🔇 Additional comments (45)
crates/api_admin/src/entities/mod.rs (4)
1-1: LGTM: New module for blocking functionality.The addition of the
block_url_or_acctmodule aligns well with the PR objectives to implement blocking functionality for instances and actors.
5-5: Existing account management exports retained.The exports from the
create_remove_accountmodule are preserved, maintaining the existing account management functionality alongside the new blocking feature.
1-5: Overall structure looks good.The changes to this module file are concise and well-organized. They successfully introduce the new blocking functionality while maintaining existing features. The clear separation of concerns between different modules (blocking and account management) is commendable.
4-4: LGTM: New exports for blocking functionality.The exports of
BlockUrlQueryandBlockUrlResultfrom theblock_url_or_acctmodule align with the PR objectives. These entities likely represent the query structure and result type for the blocking API endpoints.To ensure the correct implementation of these entities, please run the following script:
✅ Verification successful
Verified:
BlockUrlQueryandBlockUrlResultstructs are correctly exported.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of BlockUrlQuery and BlockUrlResult # Test: Check the implementation of BlockUrlQuery and BlockUrlResult ast-grep --lang rust --pattern $'struct BlockUrlQuery { $$$ } struct BlockUrlResult { $$$ }'Length of output: 584
Script:
#!/bin/bash # Description: Verify the implementation of BlockUrlQuery and BlockUrlResult # Test: Check the implementation of BlockUrlQuery ast-grep --lang rust --pattern $'struct BlockUrlQuery { $$$ }' # Test: Check the implementation of BlockUrlResult ast-grep --lang rust --pattern $'struct BlockUrlResult { $$$ }'Length of output: 132
Script:
#!/bin/bash # Description: Verify the existence of BlockUrlQuery and BlockUrlResult structs # Test: Check the implementation of BlockUrlQuery with a flexible pattern ast-grep --lang rust --pattern $'struct BlockUrlQuery { $$$ }' # Test: Check the implementation of BlockUrlResult with a flexible pattern ast-grep --lang rust --pattern $'struct BlockUrlResult { $$$ }' # Alternative Test: Use ripgrep to find struct declarations if ast-grep fails rg 'struct\s+BlockUrlQuery' || echo "BlockUrlQuery struct not found." rg 'struct\s+BlockUrlResult' || echo "BlockUrlResult struct not found."Length of output: 346
crates/apub/src/activities/db_activity_impl.rs (1)
9-9: Improved flexibility, but consider performance and documentation.The change from
serde_json::from_str(&self.activity)toserde_json::from_value(self.activity.clone())is a good improvement:
- It aligns better with the method name
into_json, as it's now converting from aValueto aValue.- It provides more flexibility by handling
Valuetypes directly instead of requiring a string.However, there are a few points to consider:
The
clone()operation could be costly for large objects. If performance is critical, you might want to explore ways to avoid the clone, such as using references or consumingself.activity.The comment "转换为 JSON" (Convert to JSON) is now less accurate. Consider updating it to reflect that we're ensuring the activity is a valid JSON value.
Consider updating the comment to:
// Ensure the activity is a valid JSON valueLet's verify the impact of this change on the rest of the codebase:
This will help ensure that all usages of
ApubActivityandinto_json()are compatible with the new implementation.✅ Verification successful
Verification Successful: No issues detected.
All instances of
ApubActivityand.into_json()are compatible with the updatedfrom_valueimplementation. The change enhances flexibility without introducing any compatibility or performance issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other usages of ApubActivity and its into_json method # Search for ApubActivity usage echo "Searching for ApubActivity usage:" rg --type rust "ApubActivity" -C 3 # Search for into_json method calls echo "\nSearching for into_json method calls:" rg --type rust "\.into_json\(\)" -C 3Length of output: 7125
Script:
#!/bin/bash # Description: Inspect ApubActivity construction and ensure compatibility with from_value # Define an array of files to inspect based on previous shell script output FILES=( "crates/api_apub/src/users/user_outbox.rs" "crates/cron/src/tasks/check_feed_item.rs" "crates/api_apub/src/activities/activity.rs" ) # Iterate over each file and search for ApubActivity constructions for FILE in "${FILES[@]}"; do echo "Inspecting ApubActivity constructions in $FILE:" rg --type rust "ApubActivity::" "$FILE" -C 5 echo "" done # Additionally, check if any ApubActivity is initialized with a string echo "Checking for ApubActivity initialized with strings:" rg --type rust "ApubActivity\s*\(\s*\".*?\"\s*\)" .Length of output: 1187
crates/db_schema/src/lib.rs (2)
6-6: LGTM! Verify the implementation of theblocked_urlmodule.The addition of the
blocked_urlmodule aligns well with the PR objectives for implementing blocking functionality. The module is correctly declared as public and placed in alphabetical order within the list of modules.To verify the implementation of the
blocked_urlmodule, run the following script:#!/bin/bash # Description: Check the contents of the blocked_url module # Display the contents of the blocked_url module echo "Contents of blocked_url module:" cat crates/db_schema/src/blocked_url.rs # Check for the presence of expected structs or functions echo "Checking for expected structures:" rg --type rust 'struct BlockedUrl|fn block_url|fn unblock_url' crates/db_schema/src/blocked_url.rs
1-1: LGTM! Verify compatibility with the rest of the project.The update to sea-orm-codegen 1.0.1 is a positive change, keeping the project up-to-date. However, it's important to ensure that this upgrade doesn't introduce any breaking changes or incompatibilities with the rest of the project.
To verify the sea-orm version used in the project and check for any potential incompatibilities, run the following script:
crates/api_admin/src/entities/block_url_or_acct.rs (1)
1-3: LGTM: Appropriate imports for the defined structs.The import statements are well-chosen and align with the requirements of the structs defined in this file. They provide necessary functionality for serialization, URL handling, and API documentation.
crates/db_schema/src/prelude.rs (2)
5-5: LGTM: BlockedUrl entity added correctlyThe addition of the
BlockedUrlentity is in line with the PR objectives for implementing blocking functionality. It's correctly placed alphabetically in the list of exports, maintaining good code organization and following the existing naming convention.
1-1: Verify compatibility with sea-orm-codegen 1.0.1The update from version 0.12.2 to 1.0.1 is a significant jump. While this change is approved, it's important to ensure that this update doesn't introduce any breaking changes or unexpected behavior in the generated code.
To verify the compatibility, please run the following script:
crates/db_schema/src/blocked_url.rs (2)
1-3: LGTM: File header and imports are correct.The file header correctly indicates that this is a generated SeaORM entity, and the necessary imports from sea_orm are present.
1-16: Overall assessment: The blocked_url entity is well-implemented and aligns with PR objectives.This new file successfully introduces the
blocked_urlentity, which is crucial for implementing the instance and actor blocking feature. The structure allows for storing both blocked instances and individual actors, supporting the PR's objectives.Key points:
- The
Modelstruct correctly defines the necessary fields for storing blocked URLs.- The
is_instancefield allows distinguishing between blocked instances and individual actors.- The entity is properly set up for use with SeaORM.
The implementation provides a solid foundation for the blocking feature. As the feature evolves, consider the suggestions for documentation and potential future improvements to enhance maintainability and functionality.
crates/apub/src/lib.rs (1)
6-6: Verify integration of the newutilsmodule.The addition of the
utilsmodule looks good. To ensure it's properly integrated with the rest of the crate:
- Verify that any functions or structures in the
utilsmodule are used appropriately in other parts of the crate.- Check if any existing code in other modules could benefit from being moved to the
utilsmodule for better organization.- Ensure that the
utilsmodule doesn't introduce any circular dependencies.To help verify the integration and usage of the new
utilsmodule, you can run the following script:This script will help us understand the contents of the
utilsmodule, its usage across the crate, and potential circular dependencies.crates/api_admin/Cargo.toml (1)
26-26: Approved: Addition of theurlcrate is appropriate for the PR objectives.The inclusion of the
urlcrate as a dependency is well-aligned with the PR's goal of implementing instance and actor blocking functionality. This crate will provide robust URL parsing and manipulation capabilities, which are essential for handling the various URL formats mentioned in the PR objectives (e.g., instance domains, actor IDs).Using
workspace = trueensures consistent versioning across the project, which is a good practice for maintaining dependency coherence.crates/db_schema/src/activity.rs (2)
10-10: Verify impact of changingactivityfield type fromStringtoJsonThe
activityfield in theModelstruct has been changed fromStringtoJsontype. This change suggests a shift in how activity data is stored and processed, which could have significant implications across the codebase.Please ensure that:
- All code that interacts with the
activityfield has been updated to handle theJsontype correctly.- Data migration scripts have been created to convert existing
Stringdata toJsonformat.- Any serialization/deserialization logic for the
activityfield has been updated.- API endpoints that send or receive activity data have been adjusted to work with the new
Jsonformat.Run the following script to identify potential areas that might need updates:
#!/bin/bash # Description: Find potential areas affected by the activity field type change # Test: Search for usage of the activity field rg --type rust -e 'activity.*String' -e '\.activity' -e 'activity:' # Test: Search for potential serialization/deserialization of activity rg --type rust -e 'serde.*activity' -e 'serialize.*activity' -e 'deserialize.*activity' # Test: Search for API endpoints handling activity data rg --type rust -e 'api.*activity' -e 'endpoint.*activity' -e 'route.*activity'Consider opening a separate issue to track the necessary updates across the codebase related to this change.
1-1: Verify compatibility with sea-orm-codegen 1.0.1The
sea-orm-codegenversion has been updated from 0.12.2 to 1.0.1. This is a significant version jump that might introduce breaking changes or new features.Please ensure that:
- The project's dependencies have been updated accordingly.
- Any breaking changes in sea-orm 1.0.1 have been addressed in the codebase.
- New features or improvements introduced in this version are leveraged where applicable.
Run the following script to check for any sea-orm related warnings or errors:
✅ Verification successful
Compatibility with sea-orm-codegen 1.0.1 Verified
All checks related to the
sea-orm-codegenupdate passed without issues. The project compiles successfully, and there are no warnings or errors associated withsea-orm.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for sea-orm related warnings or errors # Test: Compile the project and filter for sea-orm related messages cargo check 2>&1 | rg -i 'sea.?orm'Length of output: 4207
crates/db_schema/src/received_follow.rs (1)
11-12: Approved, but please clarify the purpose of the newtofield.The addition of the
to: Option<String>field to theModelstruct is correctly implemented. The field is properly defined as nullable and of text type.However, could you please clarify the specific purpose of this new field in the context of the blocking functionality being implemented? Understanding its role would help ensure it aligns with the PR objectives and is being used appropriately.
To verify the usage of this new field, let's check if it's being utilized in other parts of the codebase:
✅ Verification successful
Verification Complete: No usages of
tofield found.The new
to: Option<String>field has been correctly added to theModelstruct without affecting existing functionality. No references to this field were found in the codebase, confirming that it does not impact current components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of the new 'to' field in the received_follow model # Test: Search for references to 'received_follow' and 'to' field rg --type rust -e 'received_follow.*to' -e 'ReceivedFollow.*to'Length of output: 672
crates/db_migration/src/m20240926_000001_blocked_url.rs (3)
1-4: LGTM: Imports and struct definition are correct.The necessary imports from
sea_orm_migrationare present, and theMigrationstruct is correctly defined with theDeriveMigrationNamederive macro.
23-30: LGTM: Migrationdownmethod is correctly implemented.The
downmethod properly drops theBlockedUrltable, providing a clean way to revert the migration if needed.
1-39: Overall implementation aligns well with PR objectives.This migration file successfully implements the database structure required for blocking instances and actors as outlined in the PR objectives. The
BlockedUrltable with itsIdandIsInstancecolumns provides a flexible structure to store both blocked instances and individual actors.Key points:
- The
Idcolumn can store both instance domains and actor URLs.- The
IsInstanceboolean allows distinguishing between blocked instances and individual actors.- The migration is idempotent, ensuring it can be run multiple times without issues.
The implementation lays a solid foundation for the blocking functionality, enabling the creation of the API endpoints mentioned in the PR objectives (
/api/v0/admin/block_instance,/api/v0/admin/block_actor, and/api/v0/admin/block_acct).crates/db_migration/src/lib.rs (1)
12-12: LGTM! New migration for blocked URLs added correctly.The new migration module
m20240926_000001_blocked_urlhas been properly added and integrated into themigrations()function. This change aligns with the PR objectives to implement blocking functionality for instances and actors.To ensure the migration is implemented correctly, please review the contents of the new migration file:
Also applies to: 29-29
✅ Verification successful
** New migration for blocked URLs is correctly implemented and properly integrated.**
The migration file
crates/db_migration/src/m20240926_000001_blocked_url.rshas been verified and aligns with the PR objectives to implement blocking functionality for instances and actors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Display the contents of the new migration file # Test: Check if the file exists and show its contents file_path=$(fd -t f m20240926_000001_blocked_url.rs) if [ -n "$file_path" ]; then echo "Contents of $file_path:" cat "$file_path" else echo "Migration file not found. Please verify the file name and location." fiLength of output: 1338
crates/db_migration/src/m20240131_000004_activity.rs (1)
14-14: Verify the impact of changing Activity::Id from UUID to stringThe change from
uuid(Activity::Id)tostring(Activity::Id)aligns with the PR objectives of using URLs for blocking instances and actors. This modification provides more flexibility in storing identifiers, especially for URL-based IDs.However, this change may have significant implications:
- Existing data: If there's existing data in the
Activitytable, this change might cause compatibility issues.- Performance: Indexing and querying string-based primary keys might be less efficient than UUIDs.
- Code dependencies: Other parts of the system might expect
Activity::Idto be a UUID, which could lead to type mismatches or errors.Please run the following script to check for potential impacts:
Based on the results, we may need to update other parts of the codebase to handle the new string-based
Activity::Id.crates/apub/src/activities/like_or_announce/undo_like_or_announce.rs (1)
14-14: LGTM: New import for block verification.The addition of the
verify_blockedimport from theutilsmodule is appropriate for the new functionality being implemented in theverifymethod.crates/api_admin/src/routes/unblock_url.rs (7)
1-5: LGTM: Imports are appropriate and concise.The imports cover all necessary dependencies for the unblock URL functionality, including axum for web framework, sea_orm for database operations, and custom modules for app-specific types and utilities.
7-10: LGTM: Internal imports are appropriate.The internal imports bring in the necessary types (BlockUrlQuery, BlockUrlResult) and constants (TAG) required for the unblock URL functionality.
12-23: LGTM: Function documentation is comprehensive and follows OpenAPI standards.The utoipa documentation for the
unblock_urlfunction is well-structured and includes all necessary information:
- HTTP method (POST) and path
- Expected parameters (BlockUrlQuery)
- Possible responses (OK and BAD_REQUEST) with descriptions and body types
- Security requirement (api_key)
This documentation will be valuable for API consumers and maintainers.
24-32: LGTM: Function signature and initial logic are well-implemented.The
unblock_urlfunction is correctly defined as an async function with appropriate parameters. The use of#[debug_handler]is good for development purposes. The initial logic to find the blocked URL in the database is implemented correctly using sea_orm.
33-46: LGTM: Successful unblock logic is correctly implemented.The function properly handles the case when the URL to unblock is found:
- It deletes the URL from the database.
- It returns a 200 OK status with a JSON body containing the unblocked URL and a success message.
The use of
format!for the success message is appropriate and provides clear feedback.
47-52: LGTM: Error handling for non-existent URL is well-implemented.The function correctly handles the case when the URL to unblock is not found in the database:
- It returns an AppError with a BAD_REQUEST status.
- The error message includes the URL that was not found, which is helpful for debugging and user feedback.
This approach to error handling is consistent with best practices and provides clear information about the failure.
1-53: Great implementation of the unblock URL functionality!The
unblock_url.rsfile successfully implements the unblock URL functionality as described in the PR objectives. Key points:
- The function is well-documented using utoipa for OpenAPI specification.
- Error handling is robust, covering both successful and failed unblock scenarios.
- The implementation uses appropriate database operations and response formatting.
- The code is clean, well-structured, and follows Rust best practices.
This implementation effectively addresses the requirements for unblocking URLs in the Hatsu platform.
crates/apub/src/activities/following/undo_follow.rs (1)
14-14: LGTM: Import statement updated correctly.The new import statement correctly brings in the
verify_blockedfunction from theutilsmodule, which is used in the updatedverifymethod. The other imports (FollowandApubUser) are likely consolidations of existing imports.crates/db_schema/src/post.rs (2)
10-11: LGTM: Explicit column type definitionThe addition of
#[sea_orm(column_type = "Text")]to theobjectfield is a good practice. It explicitly defines the column type, which can prevent potential mismatches between the Rust type and the database column type.To ensure this change doesn't negatively impact existing data or queries, please run the following script:
✅ Verification successful
Verified: No conflicts with column type change
The addition of
#[sea_orm(column_type = "Text")]to theobjectfield does not introduce any conflicts with existing migrations or type conversions. All usages of theobjectfield are consistent with this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential impacts of the column type change # Test 1: Check if there are any existing migrations that might conflict with this change echo "Checking for existing migrations:" fd -e sql -e rs migration # Test 2: Look for any queries using the 'object' field that might be affected echo "Checking for queries using the 'object' field:" rg -i 'object.*post' --type rust # Test 3: Verify if there are any type conversions or casts related to the 'object' field echo "Checking for type conversions or casts:" rg -i 'as.*String.*object|object.*as.*String' --type rustLength of output: 1672
1-1: Verify compatibility with sea-orm-codegen 1.0.1The update from
sea-orm-codegen 0.12.2to1.0.1is a significant version jump. Please ensure that:
- All other parts of the codebase using SeaORM are compatible with this new version.
- Any necessary migrations or adjustments have been made to accommodate potential breaking changes.
- The project's dependencies have been updated accordingly in
Cargo.toml.Run the following script to check for any incompatibilities:
crates/db_schema/src/user.rs (3)
23-23: Approve: Reorderedlast_refreshed_atfieldThe reordering of the
last_refreshed_atfield doesn't affect the functionality of the struct. However, it's important to ensure that this change doesn't impact any code that might rely on the order of fields in the struct.Please verify the following:
- Check if there's any code that uses reflection or serialization/deserialization that might be affected by this reordering.
- Ensure that any ORM queries or raw SQL that might reference these fields by position are updated if necessary.
Run the following script to check for potential issues:
#!/bin/bash # Description: Check for code that might be affected by field reordering echo "Checking for serialization/deserialization usage:" rg --type rust -e 'serde' -e 'Serialize' -e 'Deserialize' -g '!crates/db_schema/**' echo "\nChecking for ORM queries or raw SQL using User model:" rg --type rust -e 'User::find' -e 'User::select' -e 'User::update' -e 'User::delete' -e 'SELECT.*FROM.*user' -e 'UPDATE.*user' -g '!crates/db_schema/**'
11-12: Approve: Added uniqueness constraint to thenamefieldAdding the
#[sea_orm(unique)]attribute to thenamefield is a good practice for ensuring data integrity. This change enforces uniqueness for user names at the database level.However, please verify the following:
- Ensure that existing data in the database complies with this new constraint.
- Update any code that creates or updates user records to handle potential uniqueness violations.
- Consider adding appropriate error handling in the API for cases where a duplicate name is attempted.
Run the following script to check for potential issues:
#!/bin/bash # Description: Check for user creation/update logic and error handling echo "Checking for user creation/update logic:" rg --type rust -e 'create.*user' -e 'update.*user' -g '!crates/db_schema/**' echo "\nChecking for error handling related to user operations:" rg --type rust -e 'Error.*user' -e 'user.*Error' -g '!crates/db_schema/**'
1-1: Verify compatibility with sea-orm-codegen 1.0.1The sea-orm-codegen version has been updated from 0.12.2 to 1.0.1, which is a significant version jump. Please ensure that:
- The rest of the codebase is compatible with this new version.
- Any necessary migrations or updates have been performed as per the upgrade guide.
- The project's dependencies have been updated accordingly.
To verify the sea-orm version compatibility across the project, run the following script:
crates/apub/src/activities/following/follow.rs (1)
26-26: LGTM: New import aligns with PR objectives.The addition of
verify_blockedimport is consistent with the PR's goal of implementing blocking functionality for instances and actors.crates/apub/src/activities/like_or_announce/like_or_announce.rs (1)
25-25: LGTM: New import for blocking functionality.The addition of
verify_blockedimport is appropriate for the new blocking feature being implemented.crates/apub/src/activities/following/accept_follow.rs (1)
51-51: Approve serialization change with a suggestion for verification.The change from
serde_json::to_string(&activity)?toserde_json::to_value(&activity)?is appropriate as it stores the activity as a structured JSON value rather than a string. This can provide more flexibility in data handling.To ensure this change doesn't negatively impact existing functionality, please verify:
- Data retrieval processes can handle the new JSON value format.
- Any code that previously expected a JSON string is updated accordingly.
Run the following script to check for potential impacts:
crates/apub/src/activities/create_or_update/note.rs (1)
18-18: LGTM: New import forverify_blockedfunction.The import is correctly placed and aligns with the new verification step added in the
verifymethod.crates/api_admin/src/routes/mod.rs (4)
16-16: ImportingBlockUrlResultinto entities.The addition of
BlockUrlResultto the import statement is appropriate and necessary for the new functionality.
18-21: Adding new modules for blocking functionality.Introducing
block_urlandunblock_urlmodules enhances the modularity and readability of the codebase. This aligns well with the PR objectives of implementing blocking features.
27-31: IncludingBlockUrlResultin OpenAPI components.Adding
BlockUrlResultto the OpenAPI schema ensures that the new endpoints are properly documented and available in the API specification.
54-57: Registering new routes for block and unblock URLs.The new routes for
block_url::block_urlandunblock_url::unblock_urlare correctly added to the router. This integrates the blocking functionality into the API as intended.crates/api_admin/src/routes/block_url.rs (1)
25-68: Functionblock_urlis well-implemented.The
block_urlfunction correctly handles the blocking of URLs with appropriate error handling and database interactions. The use of pattern matching simplifies the control flow, and the responses align with the expected HTTP status codes.
closed #16
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
utilsmodule for utility functions, including a function to verify blocked URLs.