Core: Inter-Phase Communication Endpoint for Phase Data Retrieval
#302
Conversation
WalkthroughThis change introduces functionality for retrieving previous course phase data. New DTOs and mapping functions in the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as Router
participant S as Service
participant Q as Database
participant D as DTO Converter
C->>R: GET /:uuid/course_phase_data
R->>S: getPrevPhaseDataByCoursePhaseID(uuid)
S->>Q: GetPrevCoursePhaseDataFromCore(to_course_phase_id)
S->>Q: GetPrevCoursePhaseDataResolution(to_course_phase_id)
Q-->>S: Return core & resolution data
S->>D: GetPrevCoursePhaseDataDTO(coreData, resolutions)
D-->>S: PrevCoursePhaseData DTO
S-->>R: PrevCoursePhaseData DTO
R-->>C: 200 OK with JSON response
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 3
🧹 Nitpick comments (5)
servers/core/coursePhase/coursePhaseDTO/get_course_phase_prev_data.go (1)
13-28: Consider pre-allocating the slice for better performance.The function correctly maps database model to DTO with proper error handling. For better performance with large datasets, consider pre-allocating the slice when the size is known.
- resolutionDTOs := []CoursePhaseResolution{} + resolutionDTOs := make([]CoursePhaseResolution, 0, len(resolutions))servers/core/db/query/course_phase.sql (1)
64-72: Consider adding ORDER BY clause for consistent results.The query correctly retrieves data from non-core endpoints, but lacks an ORDER BY clause which could lead to non-deterministic results when multiple records are returned.
SELECT po.dto_name, cpt.base_url, po.endpoint_path, dg.from_course_phase_id FROM phase_data_dependency_graph dg JOIN course_phase_type_phase_provided_output_dto po ON po.id = dg.from_course_phase_DTO_id JOIN course_phase_type cpt ON cpt.id = po.course_phase_type_id WHERE dg.to_course_phase_id = $1 AND po.endpoint_path <> 'core' +ORDER BY po.dto_name, dg.from_course_phase_id;servers/core/coursePhase/service_test.go (1)
151-152: Consider adding test for error cases.The test uses hardcoded UUIDs and only tests the happy path. Consider adding tests for error scenarios, such as invalid UUIDs or missing data.
// Example additional test: func (suite *CoursePhaseTestSuite) TestGetPrevPhaseDataByCoursePhaseID_InvalidID() { invalidID := uuid.New() // Random non-existent ID _, err := GetPrevPhaseDataByCoursePhaseID(suite.ctx, invalidID) suite.Assert().Error(err, "Expected error for non-existent course phase ID") }servers/core/coursePhase/service.go (1)
73-91: Service method implementation is robust but could benefit from context propagation in error logs.The implementation properly retrieves data from two sources and constructs the DTO. Error handling is comprehensive at each step.
Consider enhancing the error logging by including the coursePhaseID in the log message for better debugging:
prevCoursePhaseDataDTO, err := coursePhaseDTO.GetPrevCoursePhaseDataDTO(dataFromCore, resolutions) if err != nil { - log.Error("failed to create DTO: ", err) + log.WithFields(log.Fields{ + "coursePhaseID": coursePhaseID, + }).Error("failed to create previous course phase data DTO: ", err) return coursePhaseDTO.PrevCoursePhaseData{}, err }servers/core/database_dumps/course_phase_test.sql (1)
127-158: Test data is comprehensive but uses predictable UUIDs.The test data covers all the necessary relationships between entities, which is good for testing. However, using predictable UUIDs (all 'a's, 'b's, etc.) in test data could potentially conflict with production data.
Consider using
uuid_generate_v4()for test UUIDs or adding a clear comment that these are test-only UUIDs to avoid any confusion with production data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
servers/core/coursePhase/coursePhaseDTO/get_course_phase_prev_data.go(1 hunks)servers/core/coursePhase/coursePhaseDTO/prev_course_phase_resolution.go(1 hunks)servers/core/coursePhase/router.go(2 hunks)servers/core/coursePhase/service.go(1 hunks)servers/core/coursePhase/service_test.go(1 hunks)servers/core/database_dumps/course_phase_test.sql(1 hunks)servers/core/db/query/course_phase.sql(1 hunks)servers/core/db/sqlc/course_phase.sql.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
servers/core/coursePhase/coursePhaseDTO/get_course_phase_prev_data.go (2)
servers/core/coursePhase/coursePhaseDTO/prev_course_phase_resolution.go (2) (2)
CoursePhaseResolution(8-13)GetResolutionDTOFromDBModel(15-22)servers/core/db/sqlc/course_phase.sql.go (1) (1)
GetPrevCoursePhaseDataResolutionRow(194-199)
servers/core/coursePhase/coursePhaseDTO/prev_course_phase_resolution.go (1)
servers/core/db/sqlc/course_phase.sql.go (1) (1)
GetPrevCoursePhaseDataResolutionRow(194-199)
servers/core/coursePhase/router.go (1)
servers/core/coursePhase/service.go (1) (1)
GetPrevPhaseDataByCoursePhaseID(73-91)
servers/core/db/sqlc/course_phase.sql.go (1)
servers/core/db/sqlc/course_phase_type.sql.go (20) (20)
q(26-34)q(50-60)q(74-82)q(120-123)q(129-152)q(160-185)q(193-218)q(226-249)q(257-280)q(301-304)i(137-137)i(168-168)i(201-201)i(234-234)i(265-265)items(135-135)items(166-166)items(199-199)items(232-232)items(263-263)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: lint-clients / lint-clients
- GitHub Check: Mend Security Check
🔇 Additional comments (11)
servers/core/coursePhase/coursePhaseDTO/prev_course_phase_resolution.go (2)
8-13: LGTM! Good DTO structure with appropriate fields.The
CoursePhaseResolutionstruct is well-defined with proper field names and JSON tags.
15-22: LGTM! Clear mapping between DB model and DTO.The function correctly maps fields from the database model to the DTO, maintaining consistent naming conventions.
servers/core/coursePhase/coursePhaseDTO/get_course_phase_prev_data.go (1)
8-11: LGTM! Well-designed DTO structure.The
PrevCoursePhaseDatastruct is well-structured with appropriate field types and JSON tags.servers/core/db/query/course_phase.sql (2)
45-45: LGTM! Good filtering condition to exclude core endpoint paths.The added condition properly excludes records with endpoint path equal to 'core', which helps filter the results as required.
49-62: LGTM! Well-structured query to retrieve course phase data.The new query effectively retrieves and aggregates JSON data from the core endpoint, with appropriate joins and filtering conditions.
servers/core/coursePhase/service_test.go (1)
149-174: LGTM! Good test coverage for previous phase data retrieval.The test properly validates both core data and resolution data, ensuring the functionality works as expected.
servers/core/coursePhase/router.go (2)
18-18: Endpoint naming is clear and follows RESTful conventions.The endpoint path is descriptive and adheres to the RESTful resource naming convention used elsewhere in the codebase. The permission middleware correctly restricts access to admin, lecturer, and editor roles, which is appropriate for retrieving course phase data.
134-148: Handler implementation is well-structured and follows established patterns.The function correctly:
- Parses the UUID parameter with proper error handling
- Calls the appropriate service method
- Returns the data or error response using consistent patterns
The implementation matches the pattern used by other handlers in this file, making the codebase consistent and maintainable.
servers/core/database_dumps/course_phase_test.sql (1)
123-124:❓ Verification inconclusive
Potential issue with
base_urldefault value.Setting a hard-coded example URL as a default value in the database might be problematic if this gets deployed to production.
Verify whether this URL is meant to be an example for testing only, or if it will be used in production:
🌐 Web query:
What are best practices for storing and managing service URLs in database schemas?💡 Result:
To effectively store and manage service URLs in database schemas, follow these best practices informed by industry standards and architectural principles:
1. Use a Dedicated Service Registry Table
- Normalize URL components into separate columns (
protocol,host,port,path,query_parameters) for query flexibility and maintenance. This avoids redundancies and enables efficient updates[3][9].- Include metadata: Add columns like
service_name,version,environment(e.g., dev/staging/prod), andis_activeto streamline management[1][12].2. Version URLs Explicitly
- Embed API versioning in the URL path (e.g.,
/api/v1/resource) rather than relying on headers or parameters. This simplifies routing and client compatibility[1][5].- Store versioned endpoints as separate records with timestamps for lifecycle management[12].
3. Validate and Sanitize Inputs
- Percentage-encode special characters (e.g., spaces as
%20) to prevent parsing errors[5][10].- Apply regex validation to ensure URLs conform to expected patterns (e.g., enforce HTTPS for security)[10][16].
- Use foreign keys/constraints to link URLs to related services or resources, ensuring referential integrity[7][16].
4. Secure Sensitive Credentials
- Avoid storing credentials in URLs (e.g.,
user:password@host). Instead, use token-based authentication or reference credentials via environment variables[5][8].- For legacy systems requiring credentials, encrypt the URL field and restrict access using database permissions[5][11].
5. Optimize for Performance
- Index frequently queried columns like
hostorpathto accelerate lookups[7][16].- For read-heavy workloads, denormalize commonly accessed URL components (e.g.,
full_urlas a generated column)[4][9].6. Decouple Schema from Code Changes
- Maintain backward compatibility by treating URLs as immutable once published. Deprecate old versions instead of modifying existing entries[12].
- Use abstraction layers (e.g., API gateways) to map logical service names to physical URLs, insulating clients from direct database dependencies[1][12].
7. Example Schema Implementation
CREATE TABLE service_endpoints ( id UUID PRIMARY KEY DEFAULT gen_random_uuid(), service_name VARCHAR(255) NOT NULL, version VARCHAR(10) NOT NULL CHECK (version ~ '^v\d+$'), protocol VARCHAR(5) NOT NULL CHECK (protocol IN ('http', 'https')), host VARCHAR(255) NOT NULL, port INT CHECK (port BETWEEN 1 AND 65535), path VARCHAR(255), query_params JSONB, environment VARCHAR(10) NOT NULL, is_active BOOLEAN DEFAULT TRUE, created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP ); -- Index for common lookup patterns CREATE INDEX idx_service_version ON service_endpoints (service_name, version); CREATE INDEX idx_host_active ON service_endpoints (host, is_active);8. URL Construction Best Practices
- Follow RESTful conventions for paths (e.g.,
/resources/{id})[1][10].- Keep URLs concise (<74 characters) to avoid truncation in logs/UIs[10].
- Use snake_case or kebab-case for path segments (e.g.,
/client-portalvs./clientPortal)[10][16].By combining structured storage with rigorous validation and abstraction layers, you ensure URLs remain secure, maintainable, and adaptable to evolving business needs.
Citations:
- 1: https://ask.metafilter.com/254086/Best-Practices-for-Database-Interfaces-on-the-Web
- 2: https://atlasgo.io/concepts/url
- 3: https://www.geopostcodes.com/blog/best-practices-storing-addresses/
- 4: https://blog.panoply.io/database-schema-design-examples
- 5: https://www.prisma.io/docs/orm/reference/connection-urls
- 6: https://dev.to/imthedeveloper/storing-user-customisations-and-settings-how-do-you-do-it-1017
- 7: https://vertabelo.com/blog/best-practices-for-database-design/
- 8: https://docs.oracle.com/en/database/oracle/sql-developer-web/22.4/sdweb/creating-self-service-schema.html
- 9: https://fullscale.io/blog/understanding-database-schema/
- 10: https://www.mtrmarketing.com/blog/url-naming-best-practices
- 11: https://docs.oracle.com/en/database/oracle/sql-developer-web/24.1/sdweb/creating-self-service-schema.html
- 12: https://docs.aws.amazon.com/whitepapers/latest/blue-green-deployments/best-practices-for-managing-data-synchronization-and-schema-changes.html
- 13: https://forum.bubble.io/t/db-schema-ideas-on-service-offerings/299398
- 14: https://learn.microsoft.com/en-us/sql/relational-databases/security/authentication-access/create-a-database-schema?view=sql-server-ver16
- 15: https://synopse.info/forum/viewtopic.php?id=3022
- 16: https://www.cockroachlabs.com/blog/database-schema-beginners-guide/
- 17: https://dbschema.com/documentation/sqlserver/
- 18: https://www.servicenow.com/community/developer-forum/understanding-viewing-database-schema/m-p/2761509
- 19: https://www.youtube.com/watch?v=tlpzureVsq8
- 20: https://community.opentext.com/it_ops_mgt/smax-suite/f/discussions/529451/smax-database-schema
Check and Confirm: Hard-Coded Default URL Value
- File: servers/core/database_dumps/course_phase_test.sql (Lines 123-124)
- Concern: The SQL snippet sets the
base_urlcolumn to the hard-coded value'http://example.com'.- Context: Industry best practices advise against embedding static, example URLs into production schemas. For production deployments, service URLs should be managed via environment-specific configurations or a dedicated service endpoints table to ensure proper validation, versioning, and security.
- Action: Please confirm whether this URL is strictly for testing (as its inclusion in a test dump file might suggest) or if it will be used in production. If it’s intended for production, consider refactoring this entry to follow best practices for dynamic, environment-aware configuration.
servers/core/db/sqlc/course_phase.sql.go (2)
160-181:❓ Verification inconclusive
SQL query looks efficient but might return null for empty results.
The SQL query for retrieving core data uses
jsonb_object_aggwhich can returnnullwhen there are no rows to aggregate. This could lead to inconsistent handling in the DTO mapper.Verify that the
GetPrevCoursePhaseDataDTOfunction incoursePhaseDTOpackage handles null/empty JSON objects correctly:
🏁 Script executed:
#!/bin/bash # Look for null handling in the DTO mapper grep -r "GetPrevCoursePhaseDataDTO" --include="*.go" . # Check if there's handling for null/empty JSON head -30 $(grep -l "GetPrevCoursePhaseDataDTO" --include="*.go" . | head -1)Length of output: 503
Subject: Ensure Explicit Null-Handling in DTO Mapping for Empty SQL Results
The SQL query aggregates results with
jsonb_object_agg, which returnsnullwhen no rows exist. While theGetPrevCoursePhaseDataDTOfunction (inservers/core/coursePhase/coursePhaseDTO/get_course_phase_prev_data.go) is correctly wired into the processing flow, its implementation did not reveal an explicit check or normalization of anil(or empty) JSON payload. Please confirm that the function safely converts anynilJSON (for example, by defaulting to an empty JSON object such as{}) before attempting further mapping to avoid unintended behavior in the DTO mapper.
- Location:
- SQL query in
servers/core/db/sqlc/course_phase.sql.go(lines 160–181)- DTO mapper in
servers/core/coursePhase/coursePhaseDTO/get_course_phase_prev_data.go
183-224:❓ Verification inconclusive
Resolution query implementation is consistent with existing patterns.
The implementation for fetching resolution data follows the same pattern as the existing
GetResolutionsForCoursePhasefunction, which is good for code consistency. The explicit exclusion of 'core' endpoint paths ensures proper data separation.Since this functionality is similar to the participation data retrieval, verify that there are unit tests for this new functionality:
🏁 Script executed:
#!/bin/bash # Check if there are tests for the new functionality grep -r "TestGetPrevPhaseDataByCoursePhaseID" --include="*_test.go" . grep -r "GetPrevCoursePhaseDataFromCore" --include="*_test.go" .Length of output: 252
Attention: Verify Unit Test Coverage for the "FromCore" Scenario
The resolution query implementation is consistent with our existing patterns and correctly excludes 'core' endpoint paths, ensuring proper data separation. We found a unit test—
TestGetPrevPhaseDataByCoursePhaseIDinservers/core/coursePhase/service_test.go—that confirms similar functionality is exercised. However, the grep for "GetPrevCoursePhaseDataFromCore" did not return any results. Please manually verify whether additional tests are needed to cover any "FromCore" cases or if the existing tests sufficiently encompass the new functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
servers/core/coursePhase/service.go (1)
73-93: Well-structured function with good error handling.The new
GetPrevPhaseDataByCoursePhaseIDfunction is well-implemented with proper error handling at each step. The error logging with contextual information (coursePhaseID) is particularly valuable for debugging.One potential improvement would be to consider adding a timeout to the context for the database queries to ensure they don't run indefinitely.
func GetPrevPhaseDataByCoursePhaseID(ctx context.Context, coursePhaseID uuid.UUID) (coursePhaseDTO.PrevCoursePhaseData, error) { + // Set a timeout for database queries + queryCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() - dataFromCore, err := CoursePhaseServiceSingleton.queries.GetPrevCoursePhaseDataFromCore(ctx, coursePhaseID) + dataFromCore, err := CoursePhaseServiceSingleton.queries.GetPrevCoursePhaseDataFromCore(queryCtx, coursePhaseID) if err != nil { return coursePhaseDTO.PrevCoursePhaseData{}, err } - resolutions, err := CoursePhaseServiceSingleton.queries.GetPrevCoursePhaseDataResolution(ctx, coursePhaseID) + resolutions, err := CoursePhaseServiceSingleton.queries.GetPrevCoursePhaseDataResolution(queryCtx, coursePhaseID) if err != nil { return coursePhaseDTO.PrevCoursePhaseData{}, err } prevCoursePhaseDataDTO, err := coursePhaseDTO.GetPrevCoursePhaseDataDTO(dataFromCore, resolutions) if err != nil { log.WithFields(log.Fields{ "coursePhaseID": coursePhaseID, }).Error("failed to create previous course phase data DTO: ", err) return coursePhaseDTO.PrevCoursePhaseData{}, err } return prevCoursePhaseDataDTO, nil }Note: Don't forget to import "time" if implementing this suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
servers/core/coursePhase/coursePhaseDTO/get_course_phase_prev_data.go(1 hunks)servers/core/coursePhase/service.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
servers/core/coursePhase/service.go (1)
servers/core/coursePhase/coursePhaseDTO/get_course_phase_prev_data.go (2) (2)
PrevCoursePhaseData(8-11)GetPrevCoursePhaseDataDTO(13-28)
servers/core/coursePhase/coursePhaseDTO/get_course_phase_prev_data.go (2)
servers/core/coursePhase/coursePhaseDTO/prev_course_phase_resolution.go (2) (2)
CoursePhaseResolution(8-13)GetResolutionDTOFromDBModel(15-22)servers/core/db/sqlc/course_phase.sql.go (1) (1)
GetPrevCoursePhaseDataResolutionRow(194-199)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
🔇 Additional comments (4)
servers/core/coursePhase/coursePhaseDTO/get_course_phase_prev_data.go (3)
4-5: Repository import paths should use organization instead of personal fork.The import statements are using a personal GitHub username instead of the organization's repository path.
-import ( - db "github.com/niclasheun/prompt2.0/db/sqlc" - "github.com/niclasheun/prompt2.0/meta" -) +import ( + db "github.com/ls1intum/prompt2/db/sqlc" + "github.com/ls1intum/prompt2/meta" +)
8-11: The structure design looks good.The
PrevCoursePhaseDatastruct is well-designed, with clear field names and appropriate types for representing previous course phase data and its resolutions.
13-28: Good error handling and memory optimization.This function is well-structured with proper error handling. The use of
makewith an initial capacity matching the length of the input resolutions is a good optimization that avoids unnecessary slice reallocations during the append operations.servers/core/coursePhase/service.go (1)
8-9: Repository import paths should use organization instead of personal fork.The import statements are using a personal GitHub username instead of the organization's repository path.
- "github.com/niclasheun/prompt2.0/coursePhase/coursePhaseDTO" - db "github.com/niclasheun/prompt2.0/db/sqlc" + "github.com/ls1intum/prompt2/coursePhase/coursePhaseDTO" + db "github.com/ls1intum/prompt2/db/sqlc"
niclasheun
requested a deployment
to
prompt-dev-vm
GitHub Actions
Waiting
Mtze
deleted the
286-inter-phase-communication---add-endpoints-for-phase-data-retrieval-from-previous-phases
branch
Adding the endpoint for retrieving the previous data for a given course-phase-id.
Similar to the participation data, this retrieves the data of the 'purple' connections in the course phase editor.
Summary by CodeRabbit
New Features
Bug Fixes
Tests