Workers API #2
Workers API #2
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @azaddhirajkumar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request. This PR focuses on refining the Cloudflare Workers API endpoints. The primary changes involve switching several read-heavy endpoints from using POST requests with JSON bodies to using GET requests with query parameters, which is a more conventional approach for fetching data. Additionally, the process for creating the Full Text Search (FTS) index has been moved out of the API itself and into a dedicated setup script, simplifying the API surface and clarifying the one-time nature of the index creation.
Highlights
- API Method Changes: Changed the HTTP method for
/airports/routes,/airports/airlines, and/airports/hotels/nearbyendpoints fromPOSTtoGET. - Parameter Handling: Updated the handlers and README documentation for the GET endpoints (
/airports/routes,/airports/airlines,/airports/hotels/nearby) to accept parameters (likeairportCodeorairportIdanddistance) via query parameters instead of a JSON request body. - FTS Index Creation Workflow: Removed the
/fts/index/createAPI endpoint and its corresponding handler (createFTSIndex.ts). Introduced a new bash script (scripts/create-fts-index.sh) to handle the FTS index creation as a separate, documented setup step outside the running API. - Hotel Search Logic Update: Modified the
getHotelsNearAirporthandler to fetch the airport document directly by its ID using a GET request (viagetDocumentUrl) before performing the FTS search, replacing the previous N1QL query lookup by airport code. - Documentation Updates: Updated the README.md to reflect the changes in API methods, parameter handling,
curlexamples, and added a new section detailing the FTS index setup script.
Changelog
Click here to see the changelog
- cloudflare-workers/README.md
- Updated HTTP methods for
/airports/routes,/airports/airlines, and/airports/hotels/nearbyfrom POST to GET. - Removed documentation for the
/fts/index/createendpoint. - Added a new section 'FTS Index Setup' explaining how to use the new script.
- Updated
curlexamples to use GET requests with query parameters. - Changed the required parameter for
/airports/hotels/nearbyfromairportCode(in body) toairportId(in query parameters). - Added
scripts/directory to the project structure diagram.
- Updated HTTP methods for
- cloudflare-workers/scripts/create-fts-index.sh
- Added a new bash script to create the 'hotel-geo-index' FTS index using the Couchbase Data API.
- cloudflare-workers/src/handlers/createFTSIndex.ts
- Removed the handler file for the FTS index creation endpoint.
- cloudflare-workers/src/handlers/getAirportAirlines.ts
- Changed to read
airportCodefrom query parameters (c.req.query) instead of the request body (c.req.json). - Updated the error message for missing
airportCodeto indicate it's a query parameter.
- Changed to read
- cloudflare-workers/src/handlers/getAirportRoutes.ts
- Changed to read
airportCodefrom query parameters (c.req.query) instead of the request body (c.req.json). - Updated the error message for missing
airportCodeto indicate it's a query parameter.
- Changed to read
- cloudflare-workers/src/handlers/getHotelsNearAirport.ts
- Changed to read
airportIdanddistancefrom query parameters (c.req.query) instead of the request body (c.req.json). - Updated the airport lookup logic from a N1QL query by code to a GET request by document ID.
- Added specific error handling for the case where the airport document is not found (404).
- Updated the final response structure to include the
airportId. - Changed import from
getQueryUrltogetDocumentUrl.
- Changed to read
- cloudflare-workers/src/index.ts
- Removed the import and route definition for the
createFTSIndexhandler. - Changed the HTTP methods for
/airports/hotels/nearby,/airports/routes, and/airports/airlinesfrom POST to GET.
- Removed the import and route definition for the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
POST becomes GET now,
Parameters in the URL,
Setup script runs.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces several significant improvements to the Cloudflare Workers API, primarily focusing on adhering to RESTful principles and enhancing the management of Full Text Search (FTS) index creation.
Key changes include:
- API Endpoint Refactoring: Several data retrieval endpoints (
/airports/routes,/airports/airlines,/airports/hotels/nearby) have been changed fromPOSTtoGET, utilizing query parameters instead of request bodies. This aligns better with REST conventions for idempotent operations. - FTS Index Creation: The API endpoint for creating the FTS index (
/fts/index/create) has been removed. Instead, a new bash script (scripts/create-fts-index.sh) is provided for this setup task. This separates administrative/setup concerns from the runtime API, which is a good architectural decision. - Hotel Search Enhancement: The
/airports/hotels/nearbyendpoint now uses anairportId(document key) to fetch airport details directly, rather than querying byairportCode(FAA/ICAO). This simplifies the lookup and is likely more efficient. - Documentation Updates: The
README.mdhas been thoroughly updated to reflect these changes, including new API examples and instructions for the FTS index creation script.
The code changes are generally well-implemented and improve the overall quality and design of the API. One area for improvement has been identified in the new FTS index creation script regarding error handling.
As no specific style guide was provided, this review adheres to common best practices for TypeScript, Bash, and general software engineering principles.
Summary of Findings
- Error Handling in FTS Index Creation Script: The
create-fts-index.shscript does not currently check for or handle errors from thecurlcommand used to create the FTS index. It will report success even if the underlying API call fails, which can be misleading. Adding error checking would improve the script's robustness.
Merge Readiness
The pull request introduces valuable improvements to the API. However, there is a medium-severity issue identified in the create-fts-index.sh script related to error handling. It is recommended to address this issue to ensure the script provides accurate feedback to the user. Once this is resolved, the PR should be in good shape for merging. As a reviewer, I am not authorized to approve the pull request; please ensure it undergoes further review and approval as per your team's process.
| curl -X PUT "${URL}" \ | ||
| -u "${DATA_API_USERNAME}:${DATA_API_PASSWORD}" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{ | ||
| "type": "fulltext-index", | ||
| "name": "hotel-geo-index", | ||
| "sourceType": "gocbcore", | ||
| "sourceName": "travel-sample", | ||
| "sourceParams": {}, | ||
| "planParams": { | ||
| "maxPartitionsPerPIndex": 1024, | ||
| "indexPartitions": 1 | ||
| }, | ||
| "params": { | ||
| "doc_config": { | ||
| "docid_prefix_delim": "", | ||
| "docid_regexp": "", | ||
| "mode": "scope.collection.type_field", | ||
| "type_field": "type" | ||
| }, | ||
| "mapping": { | ||
| "analysis": {}, | ||
| "default_analyzer": "standard", | ||
| "default_datetime_parser": "dateTimeOptional", | ||
| "default_field": "_all", | ||
| "default_mapping": { | ||
| "dynamic": true, | ||
| "enabled": false | ||
| }, | ||
| "default_type": "_default", | ||
| "docvalues_dynamic": false, | ||
| "index_dynamic": true, | ||
| "store_dynamic": false, | ||
| "type_field": "_type", | ||
| "types": { | ||
| "inventory.hotel": { | ||
| "dynamic": true, | ||
| "enabled": true, | ||
| "properties": { | ||
| "geo": { | ||
| "dynamic": false, | ||
| "enabled": true, | ||
| "fields": [ | ||
| { | ||
| "analyzer": "keyword", | ||
| "include_in_all": true, | ||
| "include_term_vectors": true, | ||
| "index": true, | ||
| "name": "geo", | ||
| "store": true, | ||
| "type": "geopoint" | ||
| } | ||
| ] | ||
| }, | ||
| "name": { | ||
| "dynamic": false, | ||
| "enabled": true, | ||
| "fields": [ | ||
| { | ||
| "analyzer": "standard", | ||
| "include_in_all": true, | ||
| "include_term_vectors": true, | ||
| "index": true, | ||
| "name": "name", | ||
| "store": true, | ||
| "type": "text" | ||
| } | ||
| ] | ||
| }, | ||
| "city": { | ||
| "dynamic": false, | ||
| "enabled": true, | ||
| "fields": [ | ||
| { | ||
| "analyzer": "standard", | ||
| "include_in_all": true, | ||
| "include_term_vectors": true, | ||
| "index": true, | ||
| "name": "city", | ||
| "store": true, | ||
| "type": "text" | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "store": { | ||
| "indexType": "scorch", | ||
| "segmentVersion": 15 | ||
| } | ||
| } | ||
| }' | ||
|
|
||
| echo "" | ||
| echo "FTS index creation completed!" | ||
| echo "Note: The index is being built in the background. Use the hotel search endpoint once the index is ready." |
There was a problem hiding this comment.
The script currently executes the curl command to create the FTS index but doesn't explicitly check for or handle potential failures from curl (e.g., network errors, HTTP 4xx/5xx errors from the API). If the curl command fails, the script will still proceed to print the success messages (lines 135-136), which can be misleading.
Could you enhance the script to check the exit status of the curl command and report an error if it fails? Using curl -f (or --fail) makes curl exit with an error code on HTTP server errors, which can then be checked.
This would make the script more robust and provide clearer feedback to the user if the index creation doesn't succeed.
if ! curl -f -sS -X PUT "${URL}" \
-u "${DATA_API_USERNAME}:${DATA_API_PASSWORD}" \
-H "Content-Type: application/json" \
-d '{
"type": "fulltext-index",
"name": "hotel-geo-index",
"sourceType": "gocbcore",
"sourceName": "travel-sample",
"sourceParams": {},
"planParams": {
"maxPartitionsPerPIndex": 1024,
"indexPartitions": 1
},
"params": {
"doc_config": {
"docid_prefix_delim": "",
"docid_regexp": "",
"mode": "scope.collection.type_field",
"type_field": "type"
},
"mapping": {
"analysis": {},
"default_analyzer": "standard",
"default_datetime_parser": "dateTimeOptional",
"default_field": "_all",
"default_mapping": {
"dynamic": true,
"enabled": false
},
"default_type": "_default",
"docvalues_dynamic": false,
"index_dynamic": true,
"store_dynamic": false,
"type_field": "_type",
"types": {
"inventory.hotel": {
"dynamic": true,
"enabled": true,
"properties": {
"geo": {
"dynamic": false,
"enabled": true,
"fields": [
{
"analyzer": "keyword",
"include_in_all": true,
"include_term_vectors": true,
"index": true,
"name": "geo",
"store": true,
"type": "geopoint"
}
]
},
"name": {
"dynamic": false,
"enabled": true,
"fields": [
{
"analyzer": "standard",
"include_in_all": true,
"include_term_vectors": true,
"index": true,
"name": "name",
"store": true,
"type": "text"
}
]
},
"city": {
"dynamic": false,
"enabled": true,
"fields": [
{
"analyzer": "standard",
"include_in_all": true,
"include_term_vectors": true,
"index": true,
"name": "city",
"store": true,
"type": "text"
}
]
}
}
}
}
},
"store": {
"indexType": "scorch",
"segmentVersion": 15
}
}
}'; then
echo "" >&2 # Maintain blank line before error message for readability
echo "Error: FTS index creation failed. Please check curl output, credentials, endpoint, and FTS service status." >&2
exit 1
fi
echo ""
echo "FTS index creation request submitted successfully!"
echo "Note: The index is being built in the background. Use the hotel search endpoint once the index is ready."
cloudflare-workers/README.md
Outdated
| @@ -13,12 +13,11 @@ The API provides a comprehensive Airport Information System that manages airport | |||
| - `DELETE /airports/{document_key}` - Delete an airport document | |||
|
|
|||
| ### Airport Information Queries | |||
| - `POST /airports/routes` - Find routes for a specific airport | |||
| - `POST /airports/airlines` - Find airlines that service a specific airport | |||
| - `GET /airports/routes` - Find routes for a specific airport | |||
There was a problem hiding this comment.
use quote params instead of query params
cloudflare-workers/README.md
Outdated
| - `POST /airports/routes` - Find routes for a specific airport | ||
| - `POST /airports/airlines` - Find airlines that service a specific airport | ||
| - `GET /airports/routes` - Find routes for a specific airport | ||
| - `GET /airports/airlines` - Find airlines that service a specific airport |
There was a problem hiding this comment.
use quote params instead of query params
cloudflare-workers/README.md
Outdated
|
|
||
| ### Full Text Search (FTS) Features | ||
| - `POST /fts/index/create` - Create FTS index for hotel geo-spatial search | ||
| - `POST /airports/hotels/nearby` - Find hotels near a specific airport using geo-spatial FTS | ||
| - `GET /airports/hotels/nearby` - Find hotels near a specific airport using geo-spatial FTS |
There was a problem hiding this comment.
use quote params instead of query params
|
Also don't bind the env to context const app = new Hono<{ Bindings: Env }>(); have config file and use it |
Changes done |
| ); | ||
| } | ||
| const airportCode = c.req.param('airportCode'); | ||
| const env = c.env as Env; |
There was a problem hiding this comment.
Don't get bind and get env from context have a common config file which should be handling env
There was a problem hiding this comment.
Changes done as discussed.
No description provided.