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 field values remapping with models & custom dropdown source in public dashboards #44099

Merged
merged 5 commits into from
Jun 13, 2024

Conversation

ranquild
Copy link
Contributor

@ranquild ranquild commented Jun 12, 2024

Fixes #44047
Repro https://www.loom.com/share/bd15abd6412f46d0aa778f3388169e34

I didn't manage to repro the same issue in E2E no matter what I tried. So I made API calls with data for what I reproduced locally - see the video above. The repro is not 100% correct as the model query doesn't execute. But without the fix the repro fails, which is good.

In the issue we're hitting this

return this._createFallbackField();
which creates invalid field stubs, which breaks parameter remapping logic downstream. v49 is based on MLv1 so we're fixing a MLv1 field matching logic here. To not break everything, this fix only changes field matching for parameters .

How to verify:

  • CI is green

@ranquild ranquild self-assigned this Jun 12, 2024
@ranquild ranquild added the no-backport Do not backport this PR to any branch label Jun 12, 2024
@ranquild ranquild changed the title Fix field values remapping with models & custom dropdown source in public dashboards [WIP] Fix field values remapping with models & custom dropdown source in public dashboards Jun 12, 2024
Copy link

replay-io bot commented Jun 12, 2024

Status Complete ↗︎
Commit eec80c8
Results
⚠️ 2 Flaky
2376 Passed

return dimension?.field();
}

if (isConcreteFieldReference(fieldRef)) {
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 is taken from master and adopted for MLv1

@ranquild ranquild requested review from uladzimirdev and a team June 12, 2024 23:02
@ranquild ranquild changed the title [WIP] Fix field values remapping with models & custom dropdown source in public dashboards Fix field values remapping with models & custom dropdown source in public dashboards Jun 12, 2024
@@ -99,6 +99,7 @@ class FieldInner extends Base {

// added when creating "virtual fields" that are associated with a given query
query?: StructuredQuery | NativeQuery;
_comesFromEndpoint?: boolean;
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 is an existing field which was previously untyped

return { ...parameter, hasVariableTemplateTagTarget: true };
return {
...parameter,
hasVariableTemplateTagTarget: !isDimensionTarget(target),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix from master. In case we fail to find the field, do not assume it's a native query variable.

Copy link
Contributor

@uladzimirdev uladzimirdev left a comment

Choose a reason for hiding this comment

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

🤞

@uladzimirdev
Copy link
Contributor

stress test run

@ranquild ranquild merged commit 41f3881 into release-x.49.x Jun 13, 2024
109 checks passed
@ranquild ranquild deleted the 44047-parameter-mapping-models-fix branch June 13, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/QueryingComponents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants