Feature: add cache checking middleware and APISchema #151 #159

onlyjackfrost · 2023-04-11T02:23:26Z

Description

This pull request adds a new middleware to validate cache configurations defined in the .yaml file.

Validation logic:

if cache is used in .SQL file, but no cache configuration found in .yaml file, throw error.
if cache configuration is set, the refreshTime and refreshExpression can not exist at the same time.
the time interval(variable "every") in refreshTime and refreshExpression should be a valid string that can be parse by library "ms", throw Error if not valid.

Issue ticket number

issue #151

Additional Context

This PR does not include the validation of the variable "express" in refreshExpress and the indexes configuration.
the SQL defined in the .yaml file will be validated in another place(like SQL engine).

vercel · 2023-04-11T02:23:30Z

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	Apr 17, 2023 2:26am

codecov-commenter · 2023-04-12T02:28:50Z

Codecov Report

Patch coverage: 97.91% and project coverage change: +0.05 🎉

Comparison is base (d80a255) 92.97% compared to head (3fd20a3) 93.02%.

📣 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     #159      +/-   ##
===========================================
+ Coverage    92.97%   93.02%   +0.05%     
===========================================
  Files          289      291       +2     
  Lines         4398     4444      +46     
  Branches       582      597      +15     
===========================================
+ Hits          4089     4134      +45     
  Misses         200      200              
- Partials       109      110       +1

Flag	Coverage Δ
build	`95.20% <97.77%> (+0.18%)`	⬆️
cli	`89.38% <ø> (ø)`
core	`94.00% <100.00%> (+<0.01%)`	⬆️
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% <ø> (ø)`

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

Impacted Files	Coverage Δ
...ild/src/lib/schema-parser/middleware/checkCache.ts	`97.43% <97.43%> (ø)`
...d/src/lib/schema-parser/middleware/addParameter.ts	`100.00% <100.00%> (ø)`
...uild/src/lib/schema-parser/middleware/constants.ts	`100.00% <100.00%> (ø)`
...es/build/src/lib/schema-parser/middleware/index.ts	`100.00% <100.00%> (ø)`
packages/core/src/models/artifact.ts	`91.66% <100.00%> (+0.75%)`	⬆️

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

kokokuo

Beside all comment suggestions, other is LGTM.

packages/build/src/lib/schema-parser/middleware/checkCache.ts

packages/build/test/schema-parser/middleware/checkCache.spec.ts

kokokuo · 2023-04-13T08:04:30Z

packages/build/test/schema-parser/middleware/checkCache.spec.ts

+        cacheTableName: 'test_cache',
+        sql: 'SELECT * FROM test_table',
+        refreshTime: { every: '5m' },
+        refreshExpression: { expression: '*/5,*,*,*,*', every: '5m' },


Suggest changing the value */5,*,*,*,* of "expression", because other members or new members often trace the test cases to understand the user case, when using non-relative values, it will make them confused, for example, the "expression" often use be a column value or the result from call SQL function.

Like the #151, description, the MAX(created_at) is used to get the created_at max value from the query result to check refreshes or not.

If not sure how to give the value in the test case, suggest figuring out the user scenario of #150 by discussion.

Otherwise, when writing test cases, we often use sinon to stub the value for the unnecessary part to make other members know what part is that we would like to focus on the test or use fake.js to fake the value.

packages/build/test/schema-parser/middleware/checkCache.spec.ts

kokokuo

There is a suggestion that seems you miss to fix, otherwise other fixed parts still have some minor suggestions, please check it :D

kokokuo · 2023-04-17T07:29:41Z

packages/build/src/lib/schema-parser/middleware/checkCache.ts

+    }
+    if (!isUsedCache && caches) {
+      throw new ConfigurationError(
+        `Can not configurate the cache setting in Schema ${schemas.urlPath}, {% cache %} tag not been used in SQL file.`


Misspelling for configurate, please update the word.

After discussion although the ${schemas.urlPath} may be long and make the message not easy to read, we still keep it until the cases happened ( If happened, change to sourceName ).

Otherwise, suggest adding the "..." in the ${schemas.sourceName} to highlight it, btw let us make the Schema to lowercase for consistency

kokokuo · 2023-04-17T07:30:44Z

packages/build/src/lib/schema-parser/middleware/checkCache.ts

+    // throw if cache not used in .sql file
+    if (isUsedCache && !caches) {
+      throw new ConfigurationError(
+        `{% cache %} tag was used in SQL file, but the cache configurations was not found in Schema ${schemas.urlPath}.`


For the ${schemas.urlPath}, it's the same suggestion to add a double quote on it, btw let us make the Schema to lowercase for consistency

kokokuo · 2023-04-17T07:33:06Z

packages/build/src/lib/schema-parser/middleware/checkCache.ts

+        const { refreshTime, refreshExpression } = cache;
+        if (refreshTime && refreshExpression) {
+          throw new ConfigurationError(
+            `The cache ${cache.cacheTableName} of Schema ${schemas.urlPath} is invalid: Can not configure refreshTime and refreshExpression at the same time, please pick one`


The error message is so long, suggest making it lighter,
e.g: Can not set "refreshTime" and "refreshExpression" at the same time in cache ${cache.cacheTableName} of schema ${schemas.urlPath}.

kokokuo · 2023-04-17T07:35:38Z

packages/build/src/lib/schema-parser/middleware/checkCache.ts

+        // check "every" of "refreshTime" is valid
+        let every = refreshTime?.['every'];
+        if (every) {
+          const { isValid, error } = this.checkRefreshInterval(every);
+          if (!isValid) {
+            throw new ConfigurationError(
+              `The "every" of "refreshTime" in cache ${cache.cacheTableName} of schema ${schemas.urlPath} is invalid: ${error}`
+            );
+          }
+        }
+        // check "every" of "refreshExpression" is valid
+        every = refreshExpression?.['every'];
+        if (every) {
+          const { isValid, error } = this.checkRefreshInterval(every);
+          if (!isValid) {
+            throw new ConfigurationError(
+              `The "every" of "refreshExpression" in cache ${cache.cacheTableName} of schema ${schemas.urlPath} is invalid: ${error}`
+            );
+          }
+        }


This code is similar, could we refactor it by extracting it to a private method?

The error message is better now ~ minor suggestion like above, add a double quote on ${schemas.urlPath},

kokokuo · 2023-04-17T07:36:13Z

packages/build/src/lib/schema-parser/middleware/checkCache.ts

Hi @onlyjackfrost have you missed the suggestion ?

onlyjackfrost · 2023-04-18T10:14:08Z

Close this PR due to Github does not detect the latest commit.

add cache checking middleware and APISchema

5200545

add 'every' validation check for refreshTime and refreshExpression

043a574

vercel bot deployed to Preview April 11, 2023 11:34 View deployment

onlyjackfrost changed the title ~~[Draft] Feature: add cache checking middleware and APISchema #151~~ Feature: add cache checking middleware and APISchema #151 Apr 12, 2023

onlyjackfrost requested a review from kokokuo April 12, 2023 03:12

kokokuo reviewed Apr 13, 2023

View reviewed changes

onlyjackfrost added 3 commits April 14, 2023 16:29

add refreshTime and refreshExpression schema to hint user

7836ea8

add validation logic and provide more information in error message

37ee4c1

update test case

66adacb

vercel bot deployed to Preview April 17, 2023 02:22 View deployment

capitalize test cases description

3fd20a3

vercel bot deployed to Preview April 17, 2023 02:26 View deployment

kokokuo reviewed Apr 17, 2023

View reviewed changes

onlyjackfrost closed this Apr 18, 2023

onlyjackfrost mentioned this pull request Apr 18, 2023

Feature: add cache checking middleware and APISchema #151 #161

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: add cache checking middleware and APISchema #151 #159

Feature: add cache checking middleware and APISchema #151 #159

onlyjackfrost commented Apr 11, 2023 •

edited

Loading

vercel bot commented Apr 11, 2023 •

edited

Loading

codecov-commenter commented Apr 12, 2023 •

edited

Loading

kokokuo left a comment

kokokuo Apr 13, 2023

kokokuo left a comment

kokokuo Apr 17, 2023 •

edited

Loading

kokokuo Apr 17, 2023 •

edited

Loading

kokokuo Apr 17, 2023 •

edited

Loading

kokokuo Apr 17, 2023 •

edited

Loading

kokokuo Apr 17, 2023

onlyjackfrost commented Apr 18, 2023

Feature: add cache checking middleware and APISchema #151 #159

Feature: add cache checking middleware and APISchema #151 #159

Conversation

onlyjackfrost commented Apr 11, 2023 • edited Loading

Description

Issue ticket number

Additional Context

vercel bot commented Apr 11, 2023 • edited Loading

codecov-commenter commented Apr 12, 2023 • edited Loading

Codecov Report

kokokuo left a comment

Choose a reason for hiding this comment

kokokuo Apr 13, 2023

Choose a reason for hiding this comment

kokokuo left a comment

Choose a reason for hiding this comment

kokokuo Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

kokokuo Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

kokokuo Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

kokokuo Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

kokokuo Apr 17, 2023

Choose a reason for hiding this comment

onlyjackfrost commented Apr 18, 2023

onlyjackfrost commented Apr 11, 2023 •

edited

Loading

vercel bot commented Apr 11, 2023 •

edited

Loading

codecov-commenter commented Apr 12, 2023 •

edited

Loading

kokokuo Apr 17, 2023 •

edited

Loading

kokokuo Apr 17, 2023 •

edited

Loading

kokokuo Apr 17, 2023 •

edited

Loading

kokokuo Apr 17, 2023 •

edited

Loading