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

add openapi-categorize-endpoint #165

Closed
wants to merge 5 commits into from
Closed

add openapi-categorize-endpoint #165

wants to merge 5 commits into from

Conversation

jamalsoueidan
Copy link
Owner

@jamalsoueidan jamalsoueidan commented Jun 28, 2024

PR Type

Enhancement, Dependencies


Description

  • Added a new HTTP endpoint for product categorization using OpenAI.
  • Implemented a controller to validate and handle product categorization requests.
  • Integrated OpenAI API for product categorization and matched products to existing collections.
  • Added GraphQL types for collections and collection fragments.
  • Updated code generation commands in deployment and test workflows.
  • Added new dependencies required for OpenAI integration.

Changes walkthrough 📝

Relevant files
Enhancement
openai.function.ts
Add HTTP endpoint for product categorization                         

src/functions/openai.function.ts

  • Added a new HTTP endpoint for product categorization using OpenAI.
  • Configured the endpoint to handle POST requests.
  • +10/-0   
    product-categorize.ts
    Implement product categorization controller                           

    src/functions/openai/controllers/product-categorize.ts

  • Defined request and response types for product categorization.
  • Implemented a controller to validate and handle product categorization
    requests.
  • +23/-0   
    product-categorize.ts
    Integrate OpenAI API for product categorization                   

    src/functions/openai/services/product-categorize.ts

  • Integrated OpenAI API for product categorization.
  • Implemented logic to match products to existing collections.
  • Added error handling for API requests.
  • +94/-0   
    admin.generated.d.ts
    Add GraphQL types for collections                                               

    src/types/admin.generated.d.ts

    • Added GraphQL types for collections and collection fragments.
    +14/-0   
    Configuration changes
    deploy-azure-functions-production.yml
    Update code generation command in deployment workflow       

    .github/workflows/deploy-azure-functions-production.yml

    • Updated command to run code generation script.
    +1/-1     
    test.yml
    Update code generation command in test workflow                   

    .github/workflows/test.yml

    • Updated command to run code generation script.
    +1/-1     
    Dependencies
    package-lock.json
    Add new dependencies for OpenAI integration                           

    package-lock.json

  • Added new dependencies: openai, agentkeepalive, form-data-encoder,
    formdata-node, node-domexception, humanize-ms.
  • +81/-3   
    package.json
    Update dependencies and scripts                                                   

    package.json

  • Added openai to dependencies.
  • Renamed graphql:codegen script to codegen.
  • +2/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @github-actions github-actions bot added enhancement New feature or request dependencies labels Jun 28, 2024
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 4
    🧪 Relevant tests No
    🔒 Security concerns No specific security vulnerabilities like SQL injection or XSS are introduced in this PR, but the handling of the OpenAI API key should be reviewed to ensure it is securely managed.
    ⚡ Key issues to review Possible Security Concern:
    The API key for OpenAI is accessed directly from the environment variables without any apparent encryption or secure handling. This could potentially expose sensitive information if not properly managed.
    Error Handling:
    The OpenAIServiceProductCategorize function logs errors to the console but does not seem to handle them further or propagate them. This could lead to unhandled exceptions or errors that are not visible to the caller.
    Data Validation:
    While there is some validation using Zod for the request body in OpenAIControllerProductCategorize, it's unclear if there are sufficient checks against the responses and data handling from the OpenAI API and Shopify admin requests.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Validate the presence of the OPENAI_API_KEY environment variable before usage

    It's recommended to validate the environment variable OPENAI_API_KEY before initializing
    the OpenAI instance to ensure the API key is present, which prevents runtime errors.

    src/functions/openai/services/product-categorize.ts [5-7]

    +if (!process.env.OPENAI_API_KEY) {
    +  throw new Error('OPENAI_API_KEY is not set');
    +}
     const openai = new OpenAI({
       apiKey: process.env.OPENAI_API_KEY,
     });
     
    Suggestion importance[1-10]: 10

    Why: Validating the environment variable OPENAI_API_KEY before initializing the OpenAI instance is essential to prevent runtime errors, ensuring the application does not crash due to a missing API key.

    10
    Possible bug
    Add error handling around JSON parsing to prevent crashes from malformed data

    Instead of directly using JSON.parse on potentially untrusted content, consider adding a
    try-catch block around it to handle parsing errors and avoid potential crashes.

    src/functions/openai/services/product-categorize.ts [64-65]

    -return JSON.parse(response.choices[0].message.content as any)
    -  .collections as CollectionsQuery["collections"]["nodes"];
    +try {
    +  return JSON.parse(response.choices[0].message.content as any)
    +    .collections as CollectionsQuery["collections"]["nodes"];
    +} catch (e) {
    +  console.error('Failed to parse response:', e);
    +  return [];
    +}
     
    Suggestion importance[1-10]: 10

    Why: Adding a try-catch block around JSON parsing is a critical improvement to handle potential parsing errors and avoid application crashes, enhancing the robustness of the code.

    10
    Enhancement
    Add error handling to the HTTP endpoint to improve reliability

    Consider adding error handling for the HTTP endpoint to manage exceptions or failed cases
    gracefully. This can improve the reliability and user experience of the API.

    src/functions/openai.function.ts [5-10]

     app.http("openaiProductCategorize", {
       methods: ["POST"],
       authLevel: "anonymous",
       route: "openai/products-categorize",
    -  handler: OpenAIControllerProductCategorize,
    +  handler: async (req, res) => {
    +    try {
    +      await OpenAIControllerProductCategorize(req, res);
    +    } catch (error) {
    +      res.status(500).send({ error: 'Internal Server Error' });
    +    }
    +  },
     });
     
    Suggestion importance[1-10]: 9

    Why: Adding error handling to the HTTP endpoint is crucial for improving the reliability and user experience of the API. This suggestion correctly addresses the need to manage exceptions and failed cases gracefully.

    9
    Maintainability
    Refactor to separate the logic for context preparation and content generation into distinct functions

    Refactor the function to separate concerns by extracting the logic for preparing the
    collectionsContext and the OpenAI content generation into separate functions. This
    improves readability and maintainability.

    src/functions/openai/services/product-categorize.ts [21-47]

    -const collectionsContext = JSON.stringify(collections, null, 2);
    -const content = `
    -Given the following product title and description, response with the collection titles that this product fits into. The JSON structure should be:
    -{
    -  "collections": [
    -    {
    -      id: "gid://shopify/Collection/1111",
    -      title: "example",
    -      ruleSet: {
    -        rules: [{
    -          column
    -          condition
    -        }],
    +const collectionsContext = getCollectionsContext(collections);
    +const content = generateContent(collectionsContext, title, description);
    +
    +function getCollectionsContext(collections) {
    +  return JSON.stringify(collections, null, 2);
    +}
    +
    +function generateContent(collectionsContext, title, description) {
    +  return `
    +  Given the following product title and description, response with the collection titles that this product fits into. The JSON structure should be:
    +  {
    +    "collections": [
    +      {
    +        id: "gid://shopify/Collection/1111",
    +        title: "example",
    +        ruleSet: {
    +          rules: [{
    +            column
    +            condition
    +          }],
    +        },
           },
    -    },
    -  ],
    +    ],
    +  }
    +  Where:
    +  - "collections" includes the existing collections that the product fits into based on the given list of collections.
    +  ### Existing Collections:
    +  ${collectionsContext}
    +  ### Product Details:
    +  Product Title: ${title}
    +  Product Description: ${description}
    +  If you think the product fits multiply collections, it's fine, include them all in the response.
    +  `;
     }
    -Where:
    -- "collections" includes the existing collections that the product fits into based on the given list of collections.
    -### Existing Collections:
    -${collectionsContext}
    -### Product Details:
    -Product Title: ${title}
    -Product Description: ${description}
    -If you think the product fits multiply collections, it's fine, include them all in the response.
    -`;
     
    Suggestion importance[1-10]: 7

    Why: This refactoring improves code readability and maintainability by separating concerns. While it is a good practice, it is not as critical as the other suggestions, hence a slightly lower score.

    7

    @jamalsoueidan jamalsoueidan deleted the openai-product branch June 28, 2024 23:06
    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.

    1 participant