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

Feature: Passing route information to template engine #142

Merged
merged 3 commits into from
Feb 17, 2023

Conversation

JSYOU
Copy link
Contributor

@JSYOU JSYOU commented Jan 30, 2023

Description

The route information like host, path ...etc. are useful while rendering SQL queries.

select
  *
from
  "artists"
where
  {{ context.req.method }} = "GET"
and
  {{ context.header.customParameters }} = "custom parameters"
  ......

Issue ticket number

closes #62

Additional Context

@vercel
Copy link

vercel bot commented Jan 30, 2023

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

Name Status Preview Comments Updated
vulcan-sql-document ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 14, 2023 at 5:35AM (UTC)

@JSYOU JSYOU force-pushed the feature/passing-route-information-to-template-engine branch from 8219365 to c1bdea8 Compare January 30, 2023 13:52
@JSYOU JSYOU changed the title [WIP] Feature: Passing route information to template engine Feature: Passing route information to template engine Jan 30, 2023
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.

Besides some suggestions Others LGTM 👍

packages/core/src/models/artifact.ts Outdated Show resolved Hide resolved
packages/core/src/lib/template-engine/compiler.ts Outdated Show resolved Hide resolved
packages/serve/src/lib/route/route-component/baseRoute.ts Outdated Show resolved Hide resolved
packages/doc/docs/api-building/sql-syntax.mdx Outdated Show resolved Hide resolved
......
```

For more available parameters, please refer to: [Koajs#Request](https://koajs.com/#request)
Copy link
Contributor

Choose a reason for hiding this comment

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

Another suggestion is that a sample for using sample in the api schema file, because if user would like to make the build time work with sample, he should also add the req to sample:

-- We use the req in sql file
select 
*,
{{ context.req.method }} as "req-method",
{{ context.req.url }} as "req-url"
from "artists"
where 
ConstituentID = {{ context.params.id }}
and
 {{ context.req.method }} = 'GET'
# API Schema
urlPath: /artist/:id
request:
  - fieldName: id
    fieldIn: path
    description: constituent id
    validators:
      - required
sample:
  req:
    method: GET
    url: 'localhost:3000'
  parameters:
    id: '1'
  profile: duck
profile: duck

``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @kokokuo , I found that there is already a section about set-sampler in the document. Maybe we can update this section to inform the user on how to properly configure it.

Copy link
Contributor

@kokokuo kokokuo Feb 14, 2023

Choose a reason for hiding this comment

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

Could you open an issue to record the improvement content ? cc @JSYOU

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @JSYOU adding the issue with sample, I record at here #146

@kokokuo
Copy link
Contributor

kokokuo commented Feb 6, 2023

I think we could update the labs sample for adding the context.req to make our other members know how to use it and also be a regression test sample when running labs.

Otherwise, I think we could also update the package version to [0.3.1-dev.20230205.0](https://www.npmjs.com/package/@vulcan-sql/extension-driver-duckdb/v/0.3.1-dev.20230205.0), like below:

{
  "name": "my-first-vulcan-project",
  "dependencies": {
    "@vulcan-sql/core": "0.3.1-dev.20230205.0",
    "@vulcan-sql/extension-driver-duckdb": "0.3.1-dev.20230205.0",
    "@vulcan-sql/serve": "0.3.1-dev.20230205.0"
  },
  "devDependencies": {
    "@vulcan-sql/build": "0.3.1-dev.20230205.0"
  }
}

Thanks so much!

@wwwy3y3
Copy link
Member

wwwy3y3 commented Feb 7, 2023

Otherwise, I think we could also update the package version to 0.3.1-dev.20230205.0, like below:

Since it's possible users will just copy our example code in labs, maybe we should use the latest stable version (which is 0.3.1 at this moment) ?

@kokokuo
Copy link
Contributor

kokokuo commented Feb 7, 2023

Otherwise, I think we could also update the package version to 0.3.1-dev.20230205.0, like below:

Since it's possible users will just copy our example code in labs, maybe we should use the latest stable version (which is 0.3.1 at this moment) ?

Otherwise, I think we could also update the package version to 0.3.1-dev.20230205.0, like below:

Since it's possible users will just copy our example code in labs, maybe we should use the latest stable version (which is 0.3.1 at this moment)?

Sure, we could change the stable version 0.3.1.

But actually, we also provide the vulcan-demo to users for playing, and the labs actually do not need the package versions ( I recap it after your feedback on the suggestion XD", so actually it doesn't need to update, it just a memo ), because we use the Makefile to copy the current developing environment node_modules to labs for testing, cc @JSYOU to know it.

@codecov-commenter
Copy link

Codecov Report

Base: 93.03% // Head: 92.97% // Decreases project coverage by -0.07% ⚠️

Coverage data is based on head (9c4760e) compared to base (d0332d9).
Patch coverage: 42.85% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #142      +/-   ##
===========================================
- Coverage    93.03%   92.97%   -0.07%     
===========================================
  Files          289      289              
  Lines         4393     4398       +5     
  Branches       580      582       +2     
===========================================
+ Hits          4087     4089       +2     
- Misses         199      200       +1     
- Partials       107      109       +2     
Flag Coverage Δ
build 95.01% <0.00%> (-0.17%) ⬇️
core 94.00% <25.00%> (-0.09%) ⬇️
extension-dbt 97.43% <ø> (ø)
extension-debug-tools 98.11% <ø> (ø)
extension-driver-bq 85.21% <ø> (ø)
extension-driver-duckdb 100.00% <ø> (ø)
extension-driver-pg 96.11% <ø> (ø)
extension-driver-snowflake 94.59% <ø> (ø)
integration-testing 96.15% <ø> (ø)
serve 90.04% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rc/lib/schema-parser/middleware/responseSampler.ts 95.00% <0.00%> (-5.00%) ⬇️
packages/core/src/models/artifact.ts 90.90% <ø> (ø)
...s/serve/src/lib/route/route-component/baseRoute.ts 86.66% <ø> (ø)
...c/lib/template-engine/nunjucksExecutionMetadata.ts 60.00% <25.00%> (-6.67%) ⬇️
...erve/src/lib/route/route-component/restfulRoute.ts 93.75% <100.00%> (+0.41%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JSYOU JSYOU requested a review from kokokuo February 15, 2023 03:09
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 👍

@JSYOU JSYOU merged commit e503c1d into develop Feb 17, 2023
@JSYOU JSYOU deleted the feature/passing-route-information-to-template-engine branch March 8, 2023 03:04
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.

Passing route information to template engine
4 participants