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

Fix: Fix API Parameter Type Bug Causing Undefined Error and Unintended Required Status #314

Merged
merged 2 commits into from
Oct 14, 2023

Conversation

JSYOU
Copy link
Contributor

@JSYOU JSYOU commented Oct 14, 2023

Description

This PR addresses a bug in the API definition file where specifying a parameter type would cause an error when the parameter was undefined, indirectly making it required.
The fix ensures that specifying a type for a parameter no longer affects its required status.

Issue ticket number

N/A

Additional Context

urlPath: /artist/:id
request:
  - fieldName: id
    fieldIn: path
    description: constituent id
    validators:
      - required
  - fieldName: begin_date
    fieldIn: query
    type: number
    description: begin date
    validators:
      - date
sample:
  parameters:
    id: '1'
  profile: duckdb
profile: duckdb

before:
image

after:
image

@vercel
Copy link

vercel bot commented Oct 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
vulcan-sql-document ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 14, 2023 4:15pm

…d an error when the parameter was undefined, indirectly making it required
@codecov
Copy link

codecov bot commented Oct 14, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Files Coverage Δ
...ckages/core/src/lib/utils/normalizedStringValue.ts 95.83% <100.00%> (+0.59%) ⬆️
...alidators/built-in-validators/dateTypeValidator.ts 90.47% <0.00%> (-4.53%) ⬇️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@kokokuo
Copy link
Contributor

kokokuo commented Oct 14, 2023

Hi @JSYOU,

Looks like you only solved one scenario for giving the type number, but when the user does not provide the type field, it happens when the schema.yaml created from Canner enterprise data service like the below source:

# schema

urlPath: /artist/:id
request:
  - fieldName: id
    fieldIn: path
    description: constituent id
    validators:
      - required
  - fieldName: begin_date
    fieldIn: query
    description: begin date
    validators:
      - name: date
        args:
          format: YYYY-MM-DD
sample:
  # parameters:
  #   id: '1'
  # profile: duckdb
profile: duckdb

and the sql file

select 
*
from "artists"
where
ConstituentID = {{ context.params.id }}
{% if contex.params.begin_date %}
    and BeginDate = {{ context.params.begin_date }}
{% endif %}

PS: The code generated from the labs/playground package's artists but little changed schema.yaml

it still happens the error:

The input parameter is invalid, it should be date type

截圖 2023-10-14 下午11 58 49

Copy link
Contributor

@kokokuo kokokuo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kokokuo
Copy link
Contributor

kokokuo commented Oct 14, 2023

Only date validator needs to add the checker, and the integer, uuid, and string of the validator work, so it does not need to add the undefined checker.

@kokokuo kokokuo merged commit 83211a1 into develop Oct 14, 2023
6 checks passed
@hanshino hanshino deleted the fix/api-parameter-type-required-bug branch January 31, 2024 07:39
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.

2 participants