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 response streams in template #42

Merged
merged 4 commits into from
Sep 13, 2022
Merged

Conversation

oscar60310
Copy link
Contributor

@oscar60310 oscar60310 commented Aug 31, 2022

What's happened

DataSources returned streams instead of arrays, but we haven't adjusted the template output:

{% req user %}
select * from users where id = {{ context.params.id }}
{% endreq %}
{{ user.value()[0].name }} --- this will be undefined

This PR consume the stream and download data for templates.

@oscar60310 oscar60310 force-pushed the fix/response-streams branch from 10dd8f1 to fbf2c2f Compare August 31, 2022 10:21
@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2022

Codecov Report

Base: 92.40% // Head: 92.59% // Increases project coverage by +0.19% 🎉

Coverage data is based on head (59d9e37) compared to base (5ff11a6).
Patch coverage: 97.50% of modified lines in pull request are covered.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           fix/sampler-params      #42      +/-   ##
======================================================
+ Coverage               92.40%   92.59%   +0.19%     
======================================================
  Files                     221      222       +1     
  Lines                    3040     3051      +11     
  Branches                  354      351       -3     
======================================================
+ Hits                     2809     2825      +16     
+ Misses                    169      165       -4     
+ Partials                   62       61       -1     
Flag Coverage Δ
build 94.87% <ø> (ø)
cli 91.90% <ø> (ø)
core 93.50% <97.50%> (+0.37%) ⬆️
extension-dbt 97.43% <ø> (ø)
extension-debug-tools 98.11% <ø> (ø)
integration-testing 95.00% <ø> (ø)
serve 88.78% <ø> (ø)
test-utility ∅ <ø> (∅)

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

Impacted Files Coverage Δ
...ore/src/lib/data-query/builder/dataQueryBuilder.ts 88.75% <ø> (ø)
...s/core/src/lib/template-engine/nunjucksCompiler.ts 96.82% <ø> (ø)
packages/core/src/models/extensions/dataSource.ts 100.00% <ø> (ø)
packages/core/src/lib/data-source/pg.ts 42.85% <50.00%> (ø)
...ilt-in-extensions/query-builder/executorBuilder.ts 100.00% <100.00%> (ø)
...uilt-in-extensions/query-builder/executorRunner.ts 100.00% <100.00%> (ø)
...built-in-extensions/query-builder/reqTagBuilder.ts 94.91% <100.00%> (+2.23%) ⬆️
packages/core/src/lib/utils/index.ts 100.00% <100.00%> (ø)
packages/core/src/lib/utils/streams.ts 100.00% <100.00%> (ø)
...built-in-extensions/validator/parametersChecker.ts 100.00% <0.00%> (+7.69%) ⬆️

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.

@oscar60310 oscar60310 changed the title [WIP] Fix: Fix response steams in template [WIP] Fix: Fix response streams in template Aug 31, 2022
@oscar60310 oscar60310 changed the title [WIP] Fix: Fix response streams in template Fix: Fix response streams in template Sep 1, 2022
@oscar60310 oscar60310 requested a review from kokokuo September 1, 2022 02:41
@oscar60310 oscar60310 marked this pull request as ready for review September 1, 2022 02:41
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.

Beside some question, others LGTM 👍


const { getData } = response;
const dataStream = getData();
const data = await streamToArray(dataStream);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we execute builder.value() and consume the stream to the array in the ExecutorRunner when play RuntimeExtension and it's the final result, then how could we convert the array result to json / csv format in the middleware?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This filter here doesn't affect the final query builder, the formators will receive an unchanged stream.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, thanks

new nunjucks.nodes.Symbol(node.lineno, node.colno, EXECUTE_FILTER_NAME),
args
);
replace(filter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious about replace, when we call replace what do we replace to ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a callback function to remove or replace the current node in order to modify the AST tree, which provided by visitChildren helper.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks a lot for replying to me

@oscar60310
Copy link
Contributor Author

Hi @kokokuo , all issues have been resolve, I also found another bug and fix it, 4a2fdca

@kokokuo kokokuo self-requested a review September 7, 2022 09:24
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 👍 cc @oscar60310

Base automatically changed from fix/sampler-params to develop September 13, 2022 10:41
@kokokuo kokokuo merged commit 8b8ed30 into develop Sep 13, 2022
@kokokuo kokokuo deleted the fix/response-streams branch September 13, 2022 10:42
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.

3 participants