fix(ingest/metabase): Fix for query template expressions and invalid URNs for Text Cards

[pulsar256]

Fixes #10380
Fixes #9767

Checklist

• The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
• Links to related issues (if applicable)
• Tests for the changes have been added/updated (if applicable)
• Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
• For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

[Gerrit-K]

Small improvement idea (which I assume you already indicated in the issue comment): we could theoretically put .+ into a matcher group and only replace it if it's a know parameter by matching it against card_details["parameters"].

Not sure if it's worth the effort, though, as non-replaced parameters would only cause the parser to fail and ignore the lineage for this card and there's likely no harm in globally replacing everything.

[pulsar256]

Yes, I was considering doing that and decided against it to avoid introducing additional complexity without any practical benefit so far I could see. I mean, other than being more "correct" and pedantic.

The correct place to fix that is upstream, all we can do downstream is to apply workarounds and sanitization. An upstream "fix" should be rather an feature to query the dashboard and card characteristics such as dependencies / database objects being used but that is not trivial and IMHO unlikely to be implemented any time soon.

[hsheth2]

The code looks good, but this needs some tests

[pulsar256]

The code looks good, but this needs some tests

please take a look if the approach is ok. If it is fine, i will add another set of tests for the other issue and perhaps cover more variants of the templated queries.

[pulsar256]

@hsheth2 ready for round 2.

[hsheth2]

For tests for #9767, it would be sufficient to just add some unit tests that call strip_template_expressions

For #10380, since it's a fairly small change, it would also be better to modify the existing test instead of creating an entirely new one. The duplication will be harder to manage down the line

[pulsar256]

For tests for #9767, it would be sufficient to just add some unit tests that call strip_template_expressions

For #10380, since it's a fairly small change, it would also be better to modify the existing test instead of creating an entirely new one. The duplication will be harder to manage down the line

I have no problem undoing / removing the metabase mocked responses. I have considered monkey patching the existing mocked metabase responses I found it rather problematic given that I had no access to the metabase state to (re)generate those. In the end we want to test the ingestion with response payloads as close as possible to what metabase generates instead of hand crafted or patched responses. Hence I refactored the test suite to allow for side effect free (between the individual tests) usage of independent reponse mocks and I was looking into automating the capture process as well.

Do you want me to keep the refactored fixtures based approach for the response map in place or should we go back to the global state in the test suite?

[pulsar256]

(...)

Do you want me to keep the refactored fixtures based approach for the response map in place or should we go back to the global state in the test suite?

@hsheth2 ready for the next round

[hsheth2]

The test setup changes look great

Left some tiny nits, but otherwise looks good

[hsheth2]

Suggested change

	def test_9767_templated_query_is_stripped(
	def test_strip_template_expressions(

[pulsar256]

done

[pulsar256]

@hsheth2 thanks for the review. included both suggested changes, rebased.

[pulsar256]

@hsheth2 thanks, I think the workflows need another approval. Perhaps rebasing resets that?

[hsheth2]

@pulsar256 yup I believe it does. No need to rebase/merge master - we'll merge it once CI is green.