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(server): query filter coresupport #1265

Merged
merged 12 commits into from
Nov 22, 2024
Merged

Conversation

hexaforce
Copy link
Contributor

@hexaforce hexaforce commented Nov 20, 2024

Overview

What I've done

In the following Project retrieval queries, only items with the coreSupport flag set to true will be returned:

  • GetProjects
  • GetStarredProjects
  • GetDeletedProjects

Currently, projects created in the visualizer are set with coreSupport as true.

What I haven't done

How I tested

e2e test

Which point I want you to review particularly

Memo

https://www.notion.so/eukarya/BE-Projects-query-needs-to-return-Visualizer-projects-only-12f16e0fb16580529d87c5d2a8081c5e

Summary by CodeRabbit

  • New Features

    • Enhanced project retrieval logic to include projects based on core support status.
    • Updated project removal process to ensure limited projects are unpublished before deletion.
    • Improved user search functionality with error handling for unmatched queries.
    • Added new test coverage for deleted projects by workspace.
  • Bug Fixes

    • Improved filtering logic for projects to ensure accurate retrieval based on updated criteria.
  • Chores

    • Updated dependency management for improved functionality and stability.

Copy link

coderabbitai bot commented Nov 20, 2024

Walkthrough

The changes in this pull request involve modifications across several files, primarily focusing on the configuration of the docker-compose.yml, updates to project retrieval methods in both memory and MongoDB implementations, and enhancements to related test cases. The docker-compose file has adjustments to the reearth service, while the test files reflect changes in project management logic, including the introduction of new test functions and alterations to existing ones. Dependency management in the go.mod file has also been updated to reflect new and modified dependencies.

Changes

File Path Change Summary
docker-compose.yml - Changed version declaration from single-quoted to double-quoted.
- Commented out env_file directive in reearth service.
server/e2e/gql_project_test.go - Renamed TestCreateProject to TestCreateAndGetProject, changing its functionality from creating to retrieving projects.
- Introduced callRequest function for GraphQL requests.
- Added testData function for creating test projects.
- Updated TestSortByName to reflect changes in expected project edges.
server/go.mod - Added dependency github.com/oklog/ulid v1.3.1.
- Updated github.com/reearth/reearthx from v0.0.0-20241016122433-351e92319d1c to v0.0.0-20241120060035-e4cc7e5e2643.
- Removed github.com/oklog/ulid v1.3.1 from indirect requirements.
server/internal/infrastructure/memory/project.go - Modified FindByWorkspace, FindStarredByWorkspace, and FindDeletedByWorkspace methods to include a check for CoreSupport().
server/internal/infrastructure/mongo/project.go - Updated filtering logic in FindByWorkspace, FindStarredByWorkspace, and FindDeletedByWorkspace methods to use $and with multiple $or conditions for better specificity in MongoDB queries.
web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts - Removed filtering condition based on coreSupport for filtedProjects.
- Updated handleProjectRemove to check project status and handle "limited" status appropriately.
server/internal/infrastructure/mongo/project_test.go - Added new test function TestProject_FindDeletedByWorkspace to test finding deleted projects.
- Updated TestProject_FindStarredByWorkspace to include coresupport in project documents.
server/internal/infrastructure/mongo/migration/241122131410_change_core_support.go - Introduced migration function ChangeCoreSupport to rename field coresupport to core in MongoDB documents.
server/internal/infrastructure/mongo/migration/migrations.go - Added migration entry for ChangeCoreSupport in the migrations list.
server/internal/adapter/gql/loader_user.go - Updated SearchUser method to include error handling for user search functionality.

Possibly related PRs

  • feat(server): Projects Recycle bin #1169: This PR introduces a deletion flag to the project management system, which is directly related to the changes in the main PR that modify the docker-compose.yml file for the reearth service, as both involve project management functionalities.
  • feat(server): get only what hasn't been deleted #1179: This PR modifies the project retrieval methods to filter out deleted projects, which aligns with the main PR's focus on managing project configurations, including the handling of deleted projects.
  • feat(server): query filter of core support #1231: This PR focuses on modifying project retrieval queries to ensure only projects with the coreSupport flag set to true are returned, which relates to the main PR's changes in project management configurations.
  • chore(server): revert query filter of core support #1255: This PR reverts changes made in a previous PR regarding the core support filtering, which is relevant to the main PR's focus on project management and configurations.

Suggested reviewers

  • pyshx
  • mkumbobeaty
  • airslice

🐇 In the garden of code, we play,
With projects blooming day by day.
Filters changed, tests anew,
In this patch, much to pursue!
Let's hop along, with joy and cheer,
For every change brings us near! 🌼


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?

❤️ Share
🪧 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>, please review it.
    • 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @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 using 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.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

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

netlify bot commented Nov 20, 2024

Deploy Preview for reearth-web canceled.

Name Link
🔨 Latest commit cb30c2a
🔍 Latest deploy log https://app.netlify.com/sites/reearth-web/deploys/674040b4933bb60008d3918a

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: 3

🧹 Outside diff range and nitpick comments (5)
server/internal/infrastructure/memory/project.go (1)

48-48: Consider extracting CoreSupport check into a helper method.

To improve maintainability and ensure consistent filtering across methods, consider extracting the CoreSupport check into a helper method.

+func (r *Project) isEligibleProject(p *project.Project, ws accountdomain.WorkspaceID) bool {
+    return p.Workspace() == ws && p.CoreSupport()
+}

 func (r *Project) FindByWorkspace(ctx context.Context, id accountdomain.WorkspaceID, filter repo.ProjectFilter) ([]*project.Project, *usecasex.PageInfo, error) {
     // ...
-    if d.Workspace() == id && !d.IsDeleted() && d.CoreSupport() && (filter.Keyword == nil || strings.Contains(d.Name(), *filter.Keyword)) {
+    if r.isEligibleProject(d, id) && !d.IsDeleted() && (filter.Keyword == nil || strings.Contains(d.Name(), *filter.Keyword)) {
     // ...
 }

 func (r *Project) FindStarredByWorkspace(ctx context.Context, id accountdomain.WorkspaceID) ([]*project.Project, error) {
     // ...
-    if p.Workspace() == id && p.Starred() && p.CoreSupport() {
+    if r.isEligibleProject(p, id) && p.Starred() {
     // ...
 }

 func (r *Project) FindDeletedByWorkspace(ctx context.Context, id accountdomain.WorkspaceID) ([]*project.Project, error) {
     // ...
-    if p.Workspace() == id && p.IsDeleted() && p.CoreSupport() {
+    if r.isEligibleProject(p, id) && p.IsDeleted() {
     // ...
 }

Also applies to: 96-96, 118-118

server/e2e/gql_project_test.go (4)

12-20: Consider splitting the test into separate functions for clarity

The TestCreateAndGetProject function now handles both project creation and retrieval. Splitting this into TestCreateProject and TestGetProjects can improve test clarity and maintainability.


295-297: Typo in test description

There is a typo in the description field: "test updaet" should be "test update".

Apply this diff to fix the typo:

"input": map[string]any{
	"projectId":   project2ID,
-	"description": "test updaet",
+	"description": "test update",
},

476-488: Enhance error handling in callRequest function

Currently, callRequest assumes that every GraphQL request will return a 200 OK status. Consider adding error handling to assert the response status and handle unexpected HTTP errors gracefully.

For example:

func callRequest(e *httpexpect.Expect, requestBody GraphQLRequest) *httpexpect.Object {
	response := e.POST("/api/graphql").
		WithHeader("Origin", "https://example.com").
		WithHeader("authorization", "Bearer test").
		WithHeader("X-Reearth-Debug-User", uID.String()).
		WithHeader("Content-Type", "application/json").
		WithJSON(requestBody).
		Expect()

	// Check for HTTP errors
	response.Status(http.StatusOK)

	// Check for GraphQL errors
	responseBody := response.JSON().Object()
	if errors := responseBody.Value("errors").Array(); errors.Length().Raw() > 0 {
		e.T.Fatalf("GraphQL errors: %v", errors.Raw())
	}

	return responseBody.Value("data").Object()
}

Line range hint 251-259: Add error handling for starProject function

The starProject function does not handle potential errors from the GraphQL response. Consider checking for errors and asserting the expected outcomes to make the test more robust.

Example:

func starProject(e *httpexpect.Expect, projectID string) {
	updateProjectMutation := GraphQLRequest{ /* ... */ }

	response := callRequest(e, updateProjectMutation)
	project := response.
		Value("updateProject").Object().
		Value("project").Object()

	project.
		ValueEqual("id", projectID).
		ValueEqual("starred", true)
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 79e53f9 and c4e1842.

⛔ Files ignored due to path filters (1)
  • server/go.sum is excluded by !**/*.sum
📒 Files selected for processing (6)
  • docker-compose.yml (0 hunks)
  • server/e2e/gql_project_test.go (12 hunks)
  • server/go.mod (1 hunks)
  • server/internal/infrastructure/memory/project.go (3 hunks)
  • server/internal/infrastructure/mongo/project.go (3 hunks)
  • web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (0 hunks)
💤 Files with no reviewable changes (2)
  • docker-compose.yml
  • web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts
🔇 Additional comments (9)
server/internal/infrastructure/memory/project.go (3)

96-96: LGTM! CoreSupport check correctly added to starred projects filter.

The addition of p.CoreSupport() check is consistent with the PR objectives and maintains existing functionality.


48-48: LGTM! Verify consistency with MongoDB implementation.

The addition of d.CoreSupport() check aligns with the PR objectives and is correctly combined with existing filters.

Let's verify the consistency with MongoDB implementation:

✅ Verification successful

CoreSupport filtering is consistent across implementations

The MongoDB implementation correctly filters by coreSupport in a consistent manner with the memory implementation:

  • Memory: Uses d.CoreSupport() in the filter condition
  • MongoDB: Uses {"coresupport": true} in the $or clause, with fallback for documents where the field doesn't exist

The implementations are equivalent in their filtering behavior, ensuring that only core-supported projects are returned.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if both memory and mongo implementations filter by CoreSupport consistently
# Expected: Both implementations should have similar CoreSupport filtering

# Search for FindByWorkspace implementations
echo "=== Comparing FindByWorkspace implementations ==="
rg -A 5 "FindByWorkspace.*Context" server/internal/infrastructure/

Length of output: 3386


Script:

#!/bin/bash
# Let's check the MongoDB implementation's filter conditions
echo "=== MongoDB Project Filter Implementation ==="
rg -A 10 'filter := bson.M{' server/internal/infrastructure/mongo/project.go

# Let's also check if CoreSupport is used in the MongoDB implementation
echo -e "\n=== CoreSupport Usage in MongoDB Implementation ==="
rg 'CoreSupport' server/internal/infrastructure/mongo/project.go

Length of output: 461


Script:

#!/bin/bash
# Let's try a different approach to check the MongoDB implementation's filter conditions
echo "=== MongoDB Project Filter Implementation ==="
rg -A 10 "bson.M" server/internal/infrastructure/mongo/project.go

# Also check the project model in both implementations
echo -e "\n=== Project Model Definition ==="
rg -A 5 "type Project struct" server/internal/infrastructure/

Length of output: 4203


118-118: Verify business requirement for filtering deleted projects.

While the implementation is technically correct, please confirm if filtering deleted projects by CoreSupport is an intentional business requirement. This could affect data retention or recovery scenarios for non-core projects.

Let's verify this requirement:

✅ Verification successful

CoreSupport filtering for deleted projects is consistent with the codebase design

The codebase shows that CoreSupport is a fundamental property consistently used across different project states (active, starred, and deleted). This is evidenced by:

  • Similar filtering patterns in the memory repository for different project lists:
    • Active projects: d.Workspace() == id && !d.IsDeleted() && d.CoreSupport()
    • Starred projects: p.Workspace() == id && p.Starred() && p.CoreSupport()
    • Deleted projects: p.Workspace() == id && p.IsDeleted() && p.CoreSupport()

The CoreSupport flag appears to be a core architectural decision that affects project visibility and behavior throughout its lifecycle, including:

  • Visualizer selection (switches to VisualizerCesiumBeta when CoreSupport is true)
  • Scene building and configuration
  • Project export/import functionality
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any documentation or tests that specify the behavior for deleted projects
# Expected: Find evidence that filtering deleted projects by CoreSupport is intentional

echo "=== Checking documentation ==="
rg -l "deleted.*core.*support|core.*support.*deleted" docs/ README.md

echo "=== Checking related tests ==="
rg -A 5 "TestFind.*Deleted" server/internal/infrastructure/memory/

Length of output: 328


Script:

#!/bin/bash
# Let's check for test files and implementation details

# Find test files related to project deletion
echo "=== Looking for project test files ==="
fd "project.*test.*go" server/

# Check implementation details around CoreSupport and deletion
echo "=== Checking CoreSupport usage in project files ==="
rg -B2 -A2 "CoreSupport" server/

# Check for any interfaces or contracts that might define this behavior
echo "=== Checking project interfaces ==="
rg -B2 -A2 "type.*Project.*interface" server/

Length of output: 16591

server/internal/infrastructure/mongo/project.go (2)

152-154: LGTM! Correct implementation of coreSupport filter

This implementation correctly requires coreSupport=true for deleted projects, which aligns with the PR objectives.


Line range hint 88-154: Consider data migration and backwards compatibility

The introduction of strict coreSupport=true filtering will hide existing projects that don't have this flag set. Consider the following:

  1. Do we need a data migration to set coreSupport=true for existing visualizer projects?
  2. Should we add logging or monitoring to track filtered-out projects during the transition?
  3. How do we handle API clients that might depend on seeing all projects?

Let's check for potentially affected projects:

server/go.mod (2)

26-26: Clarify the purpose of adding the ULID package.

The addition of github.com/oklog/ulid seems unrelated to the PR's objective of implementing project filtering by coreSupport. Please explain why this package is necessary.

✅ Verification successful

The ULID package is used appropriately for generating unique filenames during project export

The ULID package is being used in the ExportProject mutation to generate unique, sortable filenames for project export zip files. This is a valid use case as it ensures:

  • Unique filenames for concurrent exports
  • Timestamp-based sorting capability
  • No filename collisions
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if ULID is used in the project filtering implementation
rg -l "ulid" | grep -i "project"

Length of output: 87


Script:

#!/bin/bash
# Check the context of ULID usage in the project resolver
rg -A 5 -B 5 "ulid" server/internal/adapter/gql/resolver_mutation_project.go

Length of output: 899


30-30: Verify the necessity of the reearthx version update.

The update to github.com/reearth/reearthx from version 20241016 to 20241120 appears to be a significant change. Please confirm if this update contains changes required for the project filtering functionality.

server/e2e/gql_project_test.go (2)

144-153: Potential issue with case sensitivity in sorting

When sorting by name, the test expects the order to be "a-project", "A-project", "b-project", "B-project". Depending on the database collation or sorting implementation, this may not be consistent. Consider specifying case-insensitive sorting if required.

Check if the sorting behavior aligns with the intended case sensitivity.


41-49: Ensure test assertions cover all expected outcomes

The test currently checks for coreSupport being true and isDeleted being false. Verify that the test also confirms that no projects with coreSupport false or isDeleted true are returned, ensuring comprehensive test coverage.

Run the following script to search for projects with coreSupport false or isDeleted true:

Ensure that the search returns no results.

server/e2e/gql_project_test.go Show resolved Hide resolved
@hexaforce hexaforce force-pushed the feat/query-filter-coresupport branch from c4e1842 to 370930d Compare November 20, 2024 06:33
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: 0

🧹 Outside diff range and nitpick comments (3)
server/internal/adapter/gql/loader_user.go (1)

53-53: Style: Remove the trailing period from error message

In Go, error messages conventionally don't end with punctuation.

-	return nil, errors.New("user not found.")
+	return nil, errors.New("user not found")
server/internal/infrastructure/mongo/project.go (1)

Line range hint 88-154: Maintain consistency in filter implementation across methods

For better maintainability and consistency, all methods should use the same approach for filtering coreSupport. The implementation in FindDeletedByWorkspace using a direct "coresupport": true condition is the correct pattern that should be followed in all methods.

server/e2e/gql_project_test.go (1)

25-25: Consider extracting common GraphQL fragments.

The same GraphQL fragments are duplicated across multiple queries. Consider extracting them into constants to improve maintainability and reduce duplication.

Example refactor:

const (
    projectFragment = `
    fragment ProjectFragment on Project {
        id
        name
        description
        imageUrl
        coreSupport
        isDeleted
        __typename
    }`

    createProjectMutation = `
    mutation CreateProject($teamId: ID!, $visualizer: Visualizer!, $name: String!, $description: String!, $imageUrl: URL, $coreSupport: Boolean) {
        createProject(
            input: {teamId: $teamId, visualizer: $visualizer, name: $name, description: $description, imageUrl: $imageUrl, coreSupport: $coreSupport}
        ) {
            project {
                ...ProjectFragment
            }
        }
    }
    ${projectFragment}`
)

Also applies to: 464-464, 495-495, 511-511

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c4e1842 and 370930d.

⛔ Files ignored due to path filters (1)
  • server/go.sum is excluded by !**/*.sum
📒 Files selected for processing (7)
  • docker-compose.yml (0 hunks)
  • server/e2e/gql_project_test.go (12 hunks)
  • server/go.mod (1 hunks)
  • server/internal/adapter/gql/loader_user.go (2 hunks)
  • server/internal/infrastructure/memory/project.go (3 hunks)
  • server/internal/infrastructure/mongo/project.go (3 hunks)
  • web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (0 hunks)
💤 Files with no reviewable changes (2)
  • docker-compose.yml
  • web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • server/go.mod
  • server/internal/infrastructure/memory/project.go
🔇 Additional comments (9)
server/internal/adapter/gql/loader_user.go (3)

5-5: LGTM: Appropriate import addition

The addition of the "errors" package import is necessary for the new error handling in the SearchUser method.


47-53: Verify alignment with PR objectives

While these changes improve user search precision, they appear unrelated to the PR's main objective of filtering projects by coreSupport. Consider splitting these changes into a separate PR to maintain focused code reviews.


47-51: Verify the need for additional filtering

The current implementation performs an additional iteration to find exact matches, but this filtering might be redundant if usecase.SearchUser already returns appropriately filtered results.

✅ Verification successful

Let me gather more information about the search implementation.


Let me check the resolver and loader implementations to understand the search flow better.


Let me check the usecase package more thoroughly to find the SearchUser implementation.


Let me check the interactor package for the actual SearchUser implementation.


Based on the investigation so far, I can generate the final response:

The additional filtering in the loader is necessary and correct

The loader's implementation shows that usecase.SearchUser returns multiple results, and the loader performs exact matching to find the specific user by name or email. Without seeing the usecase implementation in the codebase (which might be in a different repository or module), we can infer that it likely performs a broader search (possibly using LIKE queries or similar) and the loader's filtering is required to ensure exact matches.

The current implementation follows a pattern where:

  1. The usecase layer performs a broader search
  2. The loader layer filters for exact matches
  3. Returns only the first exact match found
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if usecase.SearchUser already performs exact matching

# Look for SearchUser implementation
ast-grep --pattern 'func $_(ctx context.Context, nameOrEmail string) ($_, error) {
  $$$
}'

# Look for any existing tests that verify the search behavior
rg -l "TestSearchUser" --type go

Length of output: 154


Script:

#!/bin/bash
# Check the implementation in the test file first
rg -A 10 "TestSearchUser" server/e2e/gql_user_test.go

# Look for the actual search implementation in usecase layer
rg -l "SearchUser" --type go

# Look for repository layer implementation that might be doing the filtering
ast-grep --pattern 'type UserRepo interface {
  $$$
}'

Length of output: 634


Script:

#!/bin/bash
# Check the resolver implementation
rg -A 10 "func.*SearchUser" server/internal/adapter/gql/resolver_query.go

# Check the loader implementation and surrounding context
rg -A 15 "func.*SearchUser" server/internal/adapter/gql/loader_user.go

# Look for any repository or usecase implementations
rg -A 10 "SearchUser" server/internal/usecase/ || true
rg -A 10 "SearchUser" server/internal/domain/ || true

Length of output: 1239


Script:

#!/bin/bash
# Find all files in usecase directory
fd . server/internal/usecase/

# Look for user-related files in usecase
rg -l "type.*User" server/internal/usecase/

# Check interface definitions that might contain SearchUser
ast-grep --pattern 'type $_ interface {
  $$$
  SearchUser($$$)
  $$$
}'

Length of output: 6362


Script:

#!/bin/bash
# Look for SearchUser implementation in interactor files
rg -A 10 "SearchUser" server/internal/usecase/interactor/

# Check for any user-related files we might have missed
fd user server/internal/usecase/

# Look for repository interface that might handle user search
rg -A 10 "type.*User.*interface" server/internal/usecase/repo/

Length of output: 157

server/internal/infrastructure/mongo/project.go (3)

152-154: LGTM! Correct implementation of coreSupport filter

This is the correct implementation of the filter that aligns with the PR objective. The direct "coresupport": true condition ensures only visualizer projects are returned.


127-139: ⚠️ Potential issue

Critical: Apply same strict coreSupport=true filter to starred projects

The same issue exists in the starred projects filter - it allows projects where coreSupport doesn't exist, contradicting the PR objective.

Apply the same change here:

 "$and": []bson.M{
   {
     "$or": []bson.M{
       {"deleted": false},
       {"deleted": bson.M{"$exists": false}},
     },
   },
-  {
-    "$or": []bson.M{
-      {"coresupport": true},
-      {"coresupport": bson.M{"$exists": false}},
-    },
-  },
+  {"coresupport": true},
 },

Likely invalid or redundant comment.


88-100: ⚠️ Potential issue

Critical: Filter must strictly require coreSupport=true

The current implementation contradicts the PR objective of "returning only items with coreSupport flag set to true". The filter allows projects where coreSupport doesn't exist, which is incorrect according to the PR description stating that visualizer projects are explicitly created with coreSupport=true.

Apply this change to ensure only visualizer projects are returned:

 "$and": []bson.M{
   {
     "$or": []bson.M{
       {"deleted": false},
       {"deleted": bson.M{"$exists": false}},
     },
   },
-  {
-    "$or": []bson.M{
-      {"coresupport": true},
-      {"coresupport": bson.M{"$exists": false}},
-    },
-  },
+  {"coresupport": true},
 },

Likely invalid or redundant comment.

server/e2e/gql_project_test.go (3)

12-49: LGTM! Test refactoring aligns with PR objectives.

The test has been appropriately renamed and modified to verify the new coreSupport filtering requirement. The assertions at lines 45-49 effectively validate that only projects with coreSupport=true and isDeleted=false are returned.


461-488: LGTM! Well-structured helper functions improve code maintainability.

The new helper functions effectively reduce code duplication and improve test maintainability:

  • createGraphQLRequest: Encapsulates project creation request logic
  • callRequest: Standardizes GraphQL request handling and response processing

490-552: LGTM! Comprehensive test data covers all edge cases.

The test data setup effectively covers all combinations of coreSupport and isDeleted flags, ensuring thorough testing of the filtering requirements.

docker-compose.yml Outdated Show resolved Hide resolved
server/internal/adapter/gql/loader_user.go Show resolved Hide resolved
server/internal/infrastructure/memory/project.go Outdated Show resolved Hide resolved
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

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 370930d and 9a91aa7.

📒 Files selected for processing (2)
  • docker-compose.yml (1 hunks)
  • server/internal/infrastructure/mongo/project_test.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • docker-compose.yml
🔇 Additional comments (1)
server/internal/infrastructure/mongo/project_test.go (1)

154-161: LGTM! Test data properly covers coreSupport filtering.

The test data setup effectively covers the new coreSupport flag requirement:

  • Tests both true/false values for coreSupport
  • Verifies that only projects with coreSupport=true are returned
  • Maintains existing test scenarios for starred projects

Consider adding a test case where a project has starred=true but coreSupport=false to explicitly verify the combined filtering behavior.

@hexaforce hexaforce force-pushed the feat/query-filter-coresupport branch from 136ffb4 to 64bd388 Compare November 21, 2024 00:58
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: 3

🧹 Outside diff range and nitpick comments (2)
server/e2e/gql_project_test.go (2)

12-12: Add descriptive comment for the test function

The function name change from TestCreateProject to TestCreateAndGetProject doesn't fully convey its new purpose of testing the coreSupport filtering functionality.

Add a descriptive comment before the function:

+// TestCreateAndGetProject verifies that GetProjects query only returns
+// non-deleted projects with coreSupport=true
 func TestCreateAndGetProject(t *testing.T) {

461-474: Consider making coreSupport parameter required

Since the PR objective is to enforce coreSupport filtering, the coreSupport parameter should be required to prevent accidental omission.

-func createGraphQLRequest(name string, coreSupport bool) GraphQLRequest {
+func createGraphQLRequest(name string, coreSupport bool /* required */) GraphQLRequest {
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 136ffb4 and 64bd388.

⛔ Files ignored due to path filters (1)
  • server/go.sum is excluded by !**/*.sum
📒 Files selected for processing (8)
  • docker-compose.yml (1 hunks)
  • server/e2e/gql_project_test.go (12 hunks)
  • server/go.mod (1 hunks)
  • server/internal/adapter/gql/loader_user.go (2 hunks)
  • server/internal/infrastructure/memory/project.go (3 hunks)
  • server/internal/infrastructure/mongo/project.go (3 hunks)
  • server/internal/infrastructure/mongo/project_test.go (2 hunks)
  • web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts (0 hunks)
💤 Files with no reviewable changes (1)
  • web/src/beta/features/Dashboard/ContentsContainer/Projects/hooks.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • docker-compose.yml
  • server/go.mod
  • server/internal/adapter/gql/loader_user.go
  • server/internal/infrastructure/memory/project.go
  • server/internal/infrastructure/mongo/project.go
🔇 Additional comments (3)
server/internal/infrastructure/mongo/project_test.go (2)

154-161: LGTM! Test data properly validates coreSupport filtering

The test data setup has been appropriately updated to include the coreSupport flag, with a specific test case (pid5) that verifies non-coreSupport projects are filtered out. The existing assertions correctly validate this behavior.


197-224: Enhance test coverage with additional scenarios

The current test implementation only covers the basic case. The previous review comment is still valid - we need additional test cases to ensure comprehensive coverage:

  1. Verify workspace filter behavior
  2. Validate filtering of non-coreSupport projects
  3. Test empty workspace scenario
  4. Add assertion to verify the correct project ID is returned

Also, the current test case should verify that pid2 is the project being returned:

 t.Run("FindDeletedByWorkspace", func(t *testing.T) {
   got, err := r.FindDeletedByWorkspace(ctx, wid)
   assert.NoError(t, err)
   assert.Equal(t, 1, len(got))
+  assert.Equal(t, pid2, got[0].ID())
 })
server/e2e/gql_project_test.go (1)

Line range hint 392-426: LGTM! Comprehensive test coverage for deleted projects

The test effectively verifies that GetDeletedProjects only returns projects with both coreSupport=true and isDeleted=true.

Comment on lines 62 to 64
createProject(e, "a-project")
createProject(e, "b-project")
createProject(e, "A-project")
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update test projects creation to include coreSupport flag

The test creates projects without explicitly setting coreSupport=true. With the new filtering, these projects might not appear in the results.

Update the project creation calls:

-createProject(e, "a-project")
-createProject(e, "b-project")
-createProject(e, "A-project")
-createProject(e, "B-project")
+createProject(e, "a-project", true)
+createProject(e, "b-project", true)
+createProject(e, "A-project", true)
+createProject(e, "B-project", true)

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines 270 to +272
createProject(e, "project3-test")

requestBody := GraphQLRequest{
updateProjectMutation := GraphQLRequest{
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update test projects creation to include coreSupport flag

Similar to TestSortByName, these projects need coreSupport=true to be included in the query results.

Update the project creation calls:

-createProject(e, "project1-test")
-project2ID := createProject(e, "project2-test")
-createProject(e, "project3-test")
+createProject(e, "project1-test", true)
+project2ID := createProject(e, "project2-test", true)
+createProject(e, "project3-test", true)

Committable suggestion skipped: line range outside the PR's diff.

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: 3

🧹 Outside diff range and nitpick comments (2)
server/internal/infrastructure/mongo/migration/241122131410_change_core_support.go (2)

11-13: Consider using a constant for the collection name.

The collection name "project" is hardcoded. To improve maintainability and prevent potential issues if the collection name changes, consider defining it as a package-level constant.

+const projectCollection = "project"

 func ChangeCoreSupport(ctx context.Context, c DBClient) error {
-	col := c.WithCollection("project")
+	col := c.WithCollection(projectCollection)

44-52: Add success logging for better monitoring.

Consider adding a log message after successful saves to track migration progress.

 if len(ids) > 0 {
     if err := col.SaveAll(ctx, ids, newDocs); err != nil {
         return err
     }
+    log.Infofc(ctx, "migration: ChangeCoreSupport: successfully updated %d documents", len(ids))
 }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 64bd388 and abc064c.

📒 Files selected for processing (2)
  • server/internal/infrastructure/mongo/migration/241122131410_change_core_support.go (1 hunks)
  • server/internal/infrastructure/mongo/migration/migrations.go (1 hunks)
🔇 Additional comments (2)
server/internal/infrastructure/mongo/migration/241122131410_change_core_support.go (2)

14-18: LGTM! Good use of batch processing and logging.

The implementation correctly:

  • Uses batch processing to manage memory usage
  • Includes progress logging for monitoring
  • Processes all documents as required for this migration

1-53: Verify migration registration.

Ensure this migration is properly registered in the migrations map.

✅ Verification successful

Migration is properly registered

The migration ChangeCoreSupport is correctly registered in the migrations map with timestamp 241122131410.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify migration registration in migrations.go

# Check if the migration is registered with the correct timestamp
rg "241122131410.*ChangeCoreSupport" server/internal/infrastructure/mongo/migration/migrations.go

Length of output: 134

@pyshx pyshx merged commit 69b1cd7 into main Nov 22, 2024
14 checks passed
@pyshx pyshx deleted the feat/query-filter-coresupport branch November 22, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants