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

Partner plugin mock #64

Open
wants to merge 58 commits into
base: dev
Choose a base branch
from
Open

Partner plugin mock #64

wants to merge 58 commits into from

Conversation

sk263
Copy link

@sk263 sk263 commented Nov 11, 2024

Description:
This pull request introduces a basic functional partner plugin mock service to the CMB repository, intended to provide a simple implementation. The mock service simulates responses for defined partner service calls, offering static, meaningful example data.

Changes:
Added service handlers for:

  • AccommodationProductList (v1, v2)
  • AccommodationProductInfo (v1, v2)
  • AccommodationSearch (v1, v2)
  • Mint (v1, v2)
  • Validation (v1, v2)

Added caching:
This caching strategy ensures that the price used at every stage of the booking process is consistent and validated, preventing discrepancies and potential issues.

Search & Cache: When a user call the AccomodationSearch service, the AccommodationSearchResult message is cached with search_id as the key.

Validate & Cache: To validate a chosen accommodation, the search_id is sent to a validation service. This service:
Retrieves the cached search results using the search_id.
Extracts the PriceDetail from the results.
Caches the PriceDetail, this time associated with the validation_id as the key.

Mint & Retrieve: The validation_id is then passed to the "mint" service (likely the booking confirmation stage). This service retrieves the validated PriceDetail from the cache using the validation_id.


  • Error Handling introduced with somewhat useful responses (should be revisited)
  • nil values replaced with empty values for responseHeader and other proto values

Summary by CodeRabbit

  • New Features

    • Introduced new accommodation service interfaces for version 1 of the accommodation protocol.
    • Added detailed API documentation for accommodation functionalities, including product retrieval and availability search.
    • New JSON files created for structured data related to accommodation products, search queries, and validation.
    • Implemented new gRPC server methods for accommodation product info, list, and search functionalities.
    • Enhanced minting process with updated request handling and response structure.
    • Added a new ping service for handling ping requests.
  • Bug Fixes

    • Updated response structures across various methods to ensure consistency and accuracy.
  • Chores

    • Removed outdated methods and refactored the server implementation for improved modularity and clarity.

@VjeraTurk VjeraTurk requested review from evlekht and Noctunus and removed request for evlekht November 14, 2024 10:34
VjeraTurk
VjeraTurk previously approved these changes Nov 14, 2024
Copy link
Member

@Noctunus Noctunus left a comment

Choose a reason for hiding this comment

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

Did a general review and will pass to Evgenii to look over it more from the go-expert perspective.
Left some comments/questions for discussion.

Copy link

coderabbitai bot commented Dec 4, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request introduces several modifications across various files related to accommodation services in a gRPC framework. Key changes include the initialization of request headers, the addition of new JSON files for structured data, and the implementation of new server methods for handling accommodation product information, lists, and searches. Additionally, the server implementation is restructured to integrate new service versions while removing outdated methods. Documentation has been enhanced to provide comprehensive specifications for accommodation functionalities.

Changes

File Path Change Summary
examples/rpc/client.go Updated Header field in AccommodationSearchRequest from nil to a new instance of typesv1.RequestHeader.
examples/rpc/partner-plugin/docs/accommodation.md Enhanced documentation with detailed specifications for accommodation products and search functionalities.
examples/rpc/partner-plugin/docs/example-messages/accommodation_product_info.json New JSON file added for structured accommodation product information, including supplier codes and languages.
examples/rpc/partner-plugin/docs/example-messages/accommodation_search.json New JSON file added for accommodation search queries, including parameters for location and supplier codes.
examples/rpc/partner-plugin/docs/example-messages/mint.json New JSON file added for minting operations, including buyer address and validation ID.
examples/rpc/partner-plugin/docs/example-messages/validation.json New JSON structure added for validation, encapsulating a search_identifier.
examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_product_info.go New server implementation for handling accommodation product info requests, including filtering logic.
examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_product_list.go New server implementation for managing accommodation product list requests.
examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_search.go New server implementation for processing accommodation search requests with filtering and error handling.
examples/rpc/partner-plugin/handlers/accommodation/v2/accommodation_product_info.go New server implementation for version 2 of accommodation product info requests.
examples/rpc/partner-plugin/handlers/accommodation/v2/accommodation_product_list.go New server implementation for version 2 of accommodation product list requests.
examples/rpc/partner-plugin/handlers/accommodation/v2/accommodation_search.go New server implementation for version 2 of accommodation search requests.
examples/rpc/partner-plugin/handlers/book/mint/v1/mint_v1.go Updated Mint method to use mintRequest parameter and generate UUID for MintId.
examples/rpc/partner-plugin/handlers/book/mint/v2/mint_v2.go New gRPC server implementation for minting requests with enhanced response structure.
examples/rpc/partner-plugin/handlers/book/validation/v1/validation_v1.go New server implementation for validation requests with structured response handling.
examples/rpc/partner-plugin/handlers/book/validation/v2/validation_v2.go New server implementation for version 2 of validation requests.
examples/rpc/partner-plugin/handlers/mint_v2.go Removed file containing previous implementation of MintServiceV2Server.
examples/rpc/partner-plugin/handlers/ping/v1/ping_handler.go New file defining a gRPC server for handling ping requests.
examples/rpc/partner-plugin/server.go Updated server implementation to integrate new accommodation services and remove outdated methods.
examples/rpc/partner-plugin/services/data/mock_data.go New package introduced to embed properties.json for runtime access.
examples/rpc/partner-plugin/services/data/properties.json New JSON file added representing structured data for multiple properties, including detailed metadata.

Poem

In fields of code where rabbits play,
New features hop in bright array.
With headers set and docs refined,
Accommodation's joy we find!
From minting dreams to search delights,
Our services leap to new heights! 🐇✨


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.

CodeRabbit Configuration File (.coderabbit.yaml)

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

Documentation and Community

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

Copy link

@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: 24

🧹 Outside diff range and nitpick comments (8)
examples/rpc/partner-plugin/services/cache/cache.go (1)

80-85: Refactor Cleanup Logic to Avoid Duplication

The logic for cleaning up expired cache entries is duplicated across multiple methods.

Extract the cleanup logic into a shared function:

func (c *searchCache) cleanupExpiredEntriesV1() {
	now := time.Now()
	for k, item := range c.cache_v1 {
		if now.After(item.expiry) {
			delete(c.cache_v1, k)
		}
	}
}

// Use in SetV1
func (c *searchCache) SetV1(key string, results []*accommodationv1.AccommodationSearchResult) {
	c.mu.Lock()
	defer c.mu.Unlock()
	c.cleanupExpiredEntriesV1()
	// Add new entry...
}

Repeat similar refactoring for other cache types.

Also applies to: 111-116, 142-147, 173-178

examples/rpc/partner-plugin/server.go (2)

78-80: Consider Refactoring Repeated Response Header Initialization

The initialization of the ResponseHeader with a success status is repeated across multiple methods. To enhance maintainability and reduce code duplication, consider creating a helper function to generate a successful response header.

Example:

func newSuccessHeader() *typesv1.ResponseHeader {
	return &typesv1.ResponseHeader{
		Status: typesv1.StatusType_STATUS_TYPE_SUCCESS,
	}
}

Then, you can use it in your responses:

Header: newSuccessHeader(),

Also applies to: 256-258, 296-296, 357-359, 378-380, 553-555, 591-593


753-755: Remove Commented-Out Code If No Longer Needed

The accommodation V2 service registrations are commented out. If these services are deprecated or replaced by the new handlers, consider removing the commented code to keep the codebase clean.

examples/rpc/partner-plugin/docs/accommodation.md (3)

161-165: Improve clarity in date restrictions documentation.

The sentence structure is redundant and could be clearer.

-There are restrictions on the travel period. Searching accommodations for travel period outside of mentioned dates is considered invalid - meaning there are no bookings available.
+Travel period restrictions: Searches outside the following dates will return no available bookings:
🧰 Tools
🪛 LanguageTool

[style] ~161-~161: This phrase is redundant. Consider using “outside”.
Context: ...rching accommodations for travel period outside of mentioned dates is considered invalid -...

(OUTSIDE_OF)


7-27: Add language specifications to code blocks.

The documentation contains multiple code blocks without language specifications, which affects syntax highlighting and readability.

Add language specifications to the code blocks:

  • Response message examples should use json
  • Request message examples should use json

Also applies to: 60-77, 81-149, 180-234, 241-252, 257-264

🧰 Tools
🪛 Markdownlint (0.35.0)

7-7: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


41-55: Enhance mock data documentation structure.

The mock data sections use inconsistent heading levels and contain trailing punctuation.

-### Mock data service available supplier codes for testing:
+## Mock Data Service
+
+### Available Supplier Codes
🧰 Tools
🪛 Markdownlint (0.35.0)

41-41: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time

(MD001, heading-increment)


41-41: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


49-49: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)

examples/rpc/partner-plugin/services/data/properties.json (2)

40-53: Consider data privacy best practices

Even in mock data, it's recommended to:

  1. Use obviously fake email domains (e.g., @example.com)
  2. Use standard placeholder phone numbers (e.g., +1 555-0123)
  3. Consider if exact coordinates are necessary for mock data

This helps prevent accidental exposure of real contact information and maintains consistent privacy practices across development and production environments.

Also applies to: 786-799, 1029-1042


330-332: Optimize mock data size and structure

  1. The height value of 7500 pixels at line 331 appears to be a typo
  2. Room descriptions are identical across properties
  3. Language versions follow the same pattern for all properties

Consider:

  1. Fixing the image height value
  2. Reducing redundancy in descriptions while maintaining testing value
  3. Using a more compact structure for repeated data patterns
- "height": 7500,
+ "height": 750,

Also applies to: 645-682, 886-923, 1136-1173

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ade5ff3 and 4ad4bbe.

📒 Files selected for processing (22)
  • examples/rpc/client.go (1 hunks)
  • examples/rpc/partner-plugin/docs/accommodation.md (1 hunks)
  • examples/rpc/partner-plugin/docs/example-messages/accommodation_product_info.json (1 hunks)
  • examples/rpc/partner-plugin/docs/example-messages/accommodation_search.json (1 hunks)
  • examples/rpc/partner-plugin/docs/example-messages/mint.json (1 hunks)
  • examples/rpc/partner-plugin/docs/example-messages/validation.json (1 hunks)
  • examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_product_info.go (1 hunks)
  • examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_product_list.go (1 hunks)
  • examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_search.go (1 hunks)
  • examples/rpc/partner-plugin/handlers/accommodation/v2/accommodation_product_info.go (1 hunks)
  • examples/rpc/partner-plugin/handlers/accommodation/v2/accommodation_product_list.go (1 hunks)
  • examples/rpc/partner-plugin/handlers/accommodation/v2/accommodation_search.go (1 hunks)
  • examples/rpc/partner-plugin/handlers/book/mint/v1/mint_v1.go (1 hunks)
  • examples/rpc/partner-plugin/handlers/book/mint/v2/mint_v2.go (1 hunks)
  • examples/rpc/partner-plugin/handlers/book/validation/v1/validation_v1.go (1 hunks)
  • examples/rpc/partner-plugin/handlers/book/validation/v2/validation_v2.go (1 hunks)
  • examples/rpc/partner-plugin/handlers/mint_v2.go (0 hunks)
  • examples/rpc/partner-plugin/handlers/ping/v1/ping_handler.go (1 hunks)
  • examples/rpc/partner-plugin/server.go (10 hunks)
  • examples/rpc/partner-plugin/services/cache/cache.go (1 hunks)
  • examples/rpc/partner-plugin/services/data/mock_data.go (1 hunks)
  • examples/rpc/partner-plugin/services/data/properties.json (1 hunks)
💤 Files with no reviewable changes (1)
  • examples/rpc/partner-plugin/handlers/mint_v2.go
✅ Files skipped from review due to trivial changes (4)
  • examples/rpc/partner-plugin/services/data/mock_data.go
  • examples/rpc/partner-plugin/docs/example-messages/validation.json
  • examples/rpc/partner-plugin/docs/example-messages/accommodation_product_info.json
  • examples/rpc/partner-plugin/docs/example-messages/mint.json
🧰 Additional context used
🪛 LanguageTool
examples/rpc/partner-plugin/docs/accommodation.md

[style] ~161-~161: This phrase is redundant. Consider using “outside”.
Context: ...rching accommodations for travel period outside of mentioned dates is considered invalid -...

(OUTSIDE_OF)

🪛 Markdownlint (0.35.0)
examples/rpc/partner-plugin/docs/accommodation.md

5-5: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time

(MD001, heading-increment)


41-41: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time

(MD001, heading-increment)


41-41: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


49-49: Punctuation: ':'
Trailing punctuation in heading

(MD026, no-trailing-punctuation)


7-7: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


60-60: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


81-81: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


180-180: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


241-241: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


257-257: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (18)
examples/rpc/partner-plugin/services/cache/cache.go (2)

13-27: Simplify or Remove Specialized Cache Structures

The cache implementations for V1 and V2 are almost identical, differing mainly in the types used.

Consider generalizing the cache structures using Go generics (if using Go 1.18 or later) or interfaces to reduce code duplication and improve maintainability.

Also applies to: 29-43


1-196: Evaluate the Necessity of Caching for Static Mock Data

Implementing caching for static mock data may introduce unnecessary complexity.

Consider whether the caching mechanism is essential for the mock service. If real-time data changes are not expected, simplifying or omitting the cache could enhance maintainability.

examples/rpc/partner-plugin/handlers/accommodation/v2/accommodation_search.go (3)

43-46: ⚠️ Potential issue

Handle JSON Unmarshalling Error Properly

Attempting to proceed after a failed unmarshal can result in runtime errors with invalid data.

Consider returning the error:

if err != nil {
	log.Printf("Error unmarshalling properties: %v", err)
+	return nil, err
}

80-81: ⚠️ Potential issue

Correct Travel Period Validation Logic

The validation condition may not behave as expected due to operator precedence.

Adjust the condition for clarity:

- if query.TravelPeriod.GetStartDate().GetYear() != 2025 || query.TravelPeriod.GetStartDate().GetMonth() != 6 || query.TravelPeriod.GetStartDate().GetDay() <= 1 &&
-     query.TravelPeriod.GetEndDate().GetYear() != 2025 || query.TravelPeriod.GetEndDate().GetMonth() != 6 || query.TravelPeriod.GetEndDate().GetDay() >= 30 {
+ if (query.TravelPeriod.GetStartDate().GetYear() != 2025 || query.TravelPeriod.GetStartDate().GetMonth() != 6 || query.TravelPeriod.GetStartDate().GetDay() < 1) ||
+     (query.TravelPeriod.GetEndDate().GetYear() != 2025 || query.TravelPeriod.GetEndDate().GetMonth() != 6 || query.TravelPeriod.GetEndDate().GetDay() > 30) {

239-239: ⚠️ Potential issue

Check for Empty Address Slice Before Accessing

Accessing the first element of Address without checking can cause an index out of range error.

Ensure the Address slice is not empty before accessing:

if len(prop.Property.ContactInfo.Address) > 0 {
	address := prop.Property.ContactInfo.Address[0]
	// Proceed with address
} else {
	// Handle absence of address appropriately
}
examples/rpc/partner-plugin/server.go (1)

782-782: Ping Service Registration Updated Correctly

The PingServiceServer is now registered with the correct handler &handlers_ping_v1.PingServiceV1Server{}. Good job on updating the registration to use the new handler implementation.

examples/rpc/partner-plugin/handlers/ping/v1/ping_handler.go (2)

1-1: Adjust Package Name for Consistency

Consider renaming the package to handlers_ping_v1 to align with the package's directory structure and avoid potential naming conflicts with other handler packages.

Apply this change:

-package handlers
+package handlers_ping_v1

18-33: Ping Method Implementation Looks Good

The Ping method correctly implements the PingServiceServer interface, handles metadata extraction, and responds with the appropriate message.

examples/rpc/partner-plugin/handlers/book/mint/v1/mint_v1.go (2)

33-33: Utilizing UUID for MintId Generation

Using uuid.New().String() to generate a unique MintId ensures that each minting operation is uniquely identified.


36-36: ValidationId Properly Included in Response

Including ValidationId from the request in the response maintains consistency in the validation process.

examples/rpc/partner-plugin/docs/example-messages/accommodation_search.json (1)

14-21: Travel Dates Are Realistic

The travel period dates in the search queries are set to realistic future dates, which is appropriate for testing and development purposes.

Also applies to: 36-43

examples/rpc/partner-plugin/handlers/book/mint/v2/mint_v2.go (2)

33-33: Correct Use of UUID for MintId

Generating the MintId using uuid.New().String() ensures unique identifiers for each minting transaction.


37-37: ValidationId Correctly Set from Request

Including the ValidationId from the request in the response ensures proper tracking and consistency in the booking process.

examples/rpc/partner-plugin/handlers/accommodation/v2/accommodation_product_list.go (1)

24-26: ⚠️ Potential issue

Fix error handling issues (duplicate of v1)

The same error handling issues present in v1 exist here:

  1. Metadata extraction error is only logged
  2. JSON unmarshalling error is ignored
  3. SendHeader error is not checked

See the fixes suggested in the v1 review.

Also applies to: 37-40, 58-58

examples/rpc/partner-plugin/handlers/book/validation/v1/validation_v1.go (2)

26-29: ⚠️ Potential issue

Improve error handling for metadata extraction

Similar to other handlers, metadata extraction error is only logged.

See the fix suggested in the accommodation handler review.


67-67: ⚠️ Potential issue

Add error handling for SendHeader

The grpc.SendHeader call could fail but the error is not checked.

See the fix suggested in the accommodation handler review.

examples/rpc/partner-plugin/handlers/book/validation/v2/validation_v2.go (1)

18-19: LGTM! Good practice using compile-time interface check.

The use of a compile-time interface check ensures the implementation satisfies the interface contract.

examples/rpc/client.go (1)

82-82: LGTM! Proper initialization of request header.

The change ensures that the request header is properly initialized instead of being nil, which aligns with the protocol requirements.

…ation_product_info.go

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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 UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 4ad4bbe and b1c3bbe.

📒 Files selected for processing (1)
  • examples/rpc/partner-plugin/handlers/accommodation/v2/accommodation_product_info.go (1 hunks)
🔇 Additional comments (7)
examples/rpc/partner-plugin/handlers/accommodation/v2/accommodation_product_info.go (7)

1-16: LGTM: Package structure and imports are well-organized

The package structure follows good practices with clear separation of protocol-specific imports and internal dependencies.


17-20: LGTM: Clean interface implementation

Good practice using compile-time interface compliance check with var _ accommodationv2grpc.AccommodationProductInfoServiceServer = (*AccommodationProductInfoV2Server)(nil).


129-137: LGTM: Well-implemented helper function

The containsProperty function is well-documented and correctly implements duplicate checking based on supplier codes.


24-26: ⚠️ Potential issue

Improve metadata error handling

The current error handling for metadata extraction only logs the error but continues execution, which could lead to issues downstream.

if err := md.ExtractMetadata(ctx); err != nil {
    log.Print("error extracting metadata")
+   return nil, fmt.Errorf("failed to extract metadata: %w", err)
}

37-40: ⚠️ Potential issue

Improve JSON unmarshaling error handling

The error handling for JSON unmarshaling only logs the error but continues with potentially invalid data.

err := json.Unmarshal([]byte(jsonProperties), &properties)
if err != nil {
    log.Printf("Error unmarshalling properties: %v", err)
+   return nil, fmt.Errorf("failed to unmarshal properties: %w", err)
}

46-46: ⚠️ Potential issue

Check for empty supplier codes slice

The condition only checks for nil but doesn't handle empty slices.

-if req.SupplierCodes != nil {
+if len(req.SupplierCodes) > 0 {

91-95: ⚠️ Potential issue

Avoid modifying shared property data

Direct modification of property fields could have unintended side effects if the property is referenced elsewhere.

if (len(filteredDescriptions) > 0 || len(filteredRoomDescriptions) > 0) && !containsProperty(filteredProperties, property) {
+   propertyCopy := *property
+   propertyCopy.LocalizedDescriptions = filteredDescriptions
+   propertyCopy.LocalizedRoomDescriptions = filteredRoomDescriptions
+   filteredProperties = append(filteredProperties, &propertyCopy)
-   property.LocalizedDescriptions = filteredDescriptions
-   property.LocalizedRoomDescriptions = filteredRoomDescriptions
-   filteredProperties = append(filteredProperties, property)
}

sk263 and others added 3 commits December 4, 2024 10:15
…ation_product_info.go

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ation_product_list.go

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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 (1)
examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_product_list.go (1)

37-49: Enhance JSON unmarshalling error handling

While the error handling returns an appropriate response, consider adding more context to help with debugging.

Apply this diff:

 err := json.Unmarshal([]byte(jsonProperties), &properties)
 if err != nil {
-    log.Printf("Error unmarshalling properties: %v", err)
+    log.Printf("Error unmarshalling properties from mock_data.PropertiesJSON: %v", err)
     return &accommodationv1.AccommodationProductListResponse{
         Header: &typesv1.ResponseHeader{
             Status: typesv1.StatusType_STATUS_TYPE_FAILURE,
             Alerts: []*typesv1.Alert{{
-                Message: "Internal server error",
+                Message: "Failed to process accommodation properties",
                 Type:    typesv1.AlertType_ALERT_TYPE_ERROR,
             }},
         },
     }, nil
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b1c3bbe and c5192b4.

📒 Files selected for processing (2)
  • examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_product_info.go (1 hunks)
  • examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_product_list.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_product_info.go
🔇 Additional comments (4)
examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_product_list.go (4)

1-16: LGTM: Package structure and imports are well-organized.

The package name and import organization follow Go conventions, and all imports are being utilized.


17-20: LGTM: Good use of compile-time interface check.

The empty struct and interface implementation check follow Go best practices.


24-26: ⚠️ Potential issue

Improve error handling for metadata extraction

The current implementation logs the error but continues execution, which could lead to undefined behavior if metadata is required for downstream operations.

Apply this diff:

 if err := md.ExtractMetadata(ctx); err != nil {
-    log.Print("error extracting metadata")
+    log.Printf("error extracting metadata: %v", err)
+    return nil, fmt.Errorf("failed to extract metadata: %w", err)
 }

67-67: ⚠️ Potential issue

Add error handling for SendHeader

The grpc.SendHeader call could fail but the error is not checked.

Apply this diff:

-grpc.SendHeader(ctx, md.ToGrpcMD())
+if err := grpc.SendHeader(ctx, md.ToGrpcMD()); err != nil {
+    log.Printf("Failed to send header: %v", err)
+    return nil, fmt.Errorf("failed to send header: %w", err)
+}

…ation_search.go

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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: 6

🧹 Outside diff range and nitpick comments (1)
examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_search.go (1)

1-290: Add unit tests for the mock service

The code lacks unit tests which are crucial for ensuring the reliability of the mock service.

Would you like me to help generate comprehensive unit tests for this mock service? I can create test cases covering:

  • Query validation
  • Travel period validation
  • Property filtering
  • Price calculations
  • Error handling scenarios
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c5192b4 and 339fdf4.

📒 Files selected for processing (1)
  • examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_search.go (1 hunks)
🔇 Additional comments (1)
examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_search.go (1)

238-241: ⚠️ Potential issue

Add bounds checking for array access

Accessing Address[0] without checking array bounds could cause a panic.

-    var address = prop.Property.ContactInfo.Address[0]
-    if address.GeoTree.CityOrResort == geoTreeLocation.CityOrResort && address.GeoTree.Country == geoTreeLocation.Country && address.GeoTree.Region == geoTreeLocation.Region {
+    if len(prop.Property.ContactInfo.Address) == 0 {
+        continue
+    }
+    address := prop.Property.ContactInfo.Address[0]
+    if address.GeoTree != nil && address.GeoTree.CityOrResort == geoTreeLocation.CityOrResort && 
+       address.GeoTree.Country == geoTreeLocation.Country && 
+       address.GeoTree.Region == geoTreeLocation.Region {

Likely invalid or redundant comment.

…ation_search.go

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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

🧹 Outside diff range and nitpick comments (4)
examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_search.go (4)

78-92: Make travel period validation configurable

Hardcoding the valid date range to June 2025 reduces the flexibility of the mock service for testing different scenarios. Consider making the date range configurable to allow testing with various periods.


158-163: Consider making price values configurable

Hardcoding the price value to "100" limits the mock's usefulness for testing different pricing scenarios. Consider fetching prices from the mock data or making them configurable.


25-25: Use consistent naming conventions

Variable names like search_generic_params and available_properties use underscores. In Go, the convention is to use camelCase for variable names. Consider renaming them to searchGenericParams and availableProperties for consistency.

Also applies to: 96-96


27-27: Use structured logging instead of fmt.Printf

Using fmt.Printf for logging is not ideal. Consider using the log package or another structured logging library for better logging practices.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 339fdf4 and 6633a17.

📒 Files selected for processing (1)
  • examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_search.go (1 hunks)
🔇 Additional comments (4)
examples/rpc/partner-plugin/handlers/accommodation/v1/accommodation_search.go (4)

29-31: Handle error from metadata extraction

The error from md.ExtractMetadata(ctx) is logged but not returned, which may lead to processing requests with invalid metadata.


42-45: Return error after failed JSON unmarshalling

If json.Unmarshal fails, the error is logged but the function continues, potentially leading to processing invalid properties data. Consider returning the error to prevent unintended behavior.


155-155: Add nil check for query.Travellers before accessing

Accessing query.Travellers without checking if it's nil may cause a panic. Add a nil check before calling getTravellerIds(query.Travellers).


238-238: Check for empty Address slice before accessing

Accessing prop.Property.ContactInfo.Address[0] without checking if Address is non-empty may cause a panic. Ensure that the slice has at least one element before accessing.

@sk263 sk263 requested a review from evlekht December 4, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants