task: Add query transformer #588
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* feat: basic eventstore completed * fix: workflow and activity state cleanup * refactor: move triggers to models * fix: Fixed UI bug * style: fix minor comment typo * feat: Updated to use programmatic subscription with manifest file * Merge branch 'main' into APP-6781-event-based-workflows * chore: minor fix post-merge * fix: general bug fixes * fix: general bug fixes * fix: Adding support for dropping events when no matcher is found * fix: Bug fixes * fix: fixes for http bindings * Merge remote-tracking branch 'origin/APP-6781-event-based-workflows' into APP-6781-event-based-workflows * fix: precommits * fix: event naming * fix: unit tests * fix: unit tests * fix: application->server * fix: Removed the example sample app to move them to sample-apps repo * chore: Removing unneeded code * fix: unit tests * fix: minor PR fixes * chore: Updated eventstore name to be retrieved from env * fix: Minor PR fixes * fix: Minor fix self.event_subscriptions never set
abhishekagrawal-atlan
requested review from
AtMrun,
firecast and
inishchith
as code owners
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) ✅ code/snyk check is complete. No issues have been found. (View Details) |
📜 Docstring Coverage ReportRESULT: PASSED (minimum: 30.0%, actual: 72.5%) Detailed Coverage Report |
📦 Trivy Vulnerability Scan Results
Report Summary
Scan Result Details✅ No vulnerabilities found during the scan for |
📦 Trivy Secret Scan Results
Report Summary
Scan Result Details✅ No secrets found during the scan for |
|
🛠 Docs available at: https://k.atlan.dev/application-sdk/add_query_transformer |
Collaborator
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
|
🛠 Full Test Coverage Report: https://k.atlan.dev/coverage/application-sdk/pr/588 |
Contributor
There was a problem hiding this comment.
Pull Request Overview
Adds support for Atlas Query transformation and enhances event-driven workflow triggers in the FastAPI server.
- Introduces a new
Querytransformer class and its YAML template for Atlas entities. - Refactors event publishing (
EventStore.publish_event) and updates server to use Dapr-based event routing withEventWorkflowTrigger. - Updates tests to exercise the new event endpoint and adjusts project tooling (Dapr port).
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/server/fastapi/test__init__.py | Refactored event-trigger tests to use httpx client |
| pyproject.toml | Changed Dapr app port from 3000 to 8000 |
| application_sdk/transformers/query/templates/query.yaml | Added Atlas Query entity template |
| application_sdk/transformers/atlas/sql.py | Added Query transformer class |
| application_sdk/test_utils/hypothesis/strategies/server/fastapi/init.py | Switched event strategy from specific to generic |
| application_sdk/server/fastapi/models.py | Added EventWorkflowRequest/Response models |
| application_sdk/server/fastapi/init.py | Overhauled event routing, Dapr subscriptions, drop |
| application_sdk/outputs/eventstore.py | Replaced create_event with publish_event and enrich metadata |
| application_sdk/constants.py | Introduced EVENT_STORE_NAME constant |
Comments suppressed due to low confidence (4)
tests/unit/server/fastapi/test__init__.py:96
- [nitpick] The test docstring still refers to "hypothesis generated event data" although Hypothesis is no longer used; please update or remove the misleading docstring.
async def test_event_trigger_success(self):
docs/docs/concepts/server.md:28
- The description no longer mentions
event_id,event_filters, or routing via Dapr; please update to reflect the new properties ofEventWorkflowTrigger.
* `EventWorkflowTrigger`: Triggers a workflow based on incoming events
application_sdk/test_utils/hypothesis/strategies/server/fastapi/init.py:64
- Using
st.builds(Event, event_type=..., event_name=..., data={})will pass those as positional args after metadata, likely resulting in an invalid constructor call; consider supplying a defaultmetadataargument or using keyword-only builds.
Event,
application_sdk/transformers/atlas/sql.py:1244
- [nitpick] The transformer reads
obj.get("querytxt")but the field name is inconsistent with typical SQL conventions; please confirm the correct input key or rename to match the template'slongRawQueryattribute.
attributes["longRawQuery"] = obj.get("querytxt")
| start-dapr = "dapr run --enable-api-logging --log-level debug --app-id app --app-port 8000 --dapr-http-port 3500 --dapr-grpc-port 50001 --dapr-http-max-request-size 1024 --resources-path components" | ||
| start-temporal = "temporal server start-dev --db-filename /tmp/temporal.db" | ||
| start-deps.shell = "poe start-dapr & poe start-temporal &" | ||
| stop-deps.shell = "lsof -ti:3000,3500,7233,50001 | xargs kill -9 2>/dev/null || true" |
There was a problem hiding this comment.
The Dapr stop-deps command still targets port 3000, but the app now runs on 8000; update the stop-deps shell task to kill the correct port.
Suggested change
| stop-deps.shell = "lsof -ti:3000,3500,7233,50001 | xargs kill -9 2>/dev/null || true" | |
| stop-deps.shell = "lsof -ti:8000,3500,7233,50001 | xargs kill -9 2>/dev/null || true" |
| ) | ||
|
|
||
| app.include_router(workflow_router, prefix="/workflows/v1") | ||
| self.app.include_router(self.workflow_router, prefix="/workflows/v1") |
There was a problem hiding this comment.
Including the same router inside each trigger registration can lead to duplicate routes; move include_router calls out of the loop to a single centralized inclusion.
Suggested change
| self.app.include_router(self.workflow_router, prefix="/workflows/v1") |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Bug: Attribute Mapping Fails, Data Duplicates
The Query.get_attributes method duplicates data in custom_attributes. The loop intended for unmapped fields incorrectly adds fields already mapped to attributes (e.g., "querytxt", "dbname", "schemaname"). This occurs because the check k not in attributes fails: input keys are mapped to different attribute names (e.g., "longRawQuery", "defaultDatabaseQualifiedName"), so the original input keys are not found in the attributes dictionary, leading to their re-addition to custom_attributes.
application_sdk/transformers/atlas/sql.py#L1274-L1278
application-sdk/application_sdk/transformers/atlas/sql.py
Lines 1274 to 1278 in 9f0b35b
Bug: Missing Fundamental Attributes in Query Entity
The Query.get_attributes method is missing the required Atlas entity attributes 'name' and 'qualified_name'. These are fundamental attributes consistently set by all other entity transformers (Database, Schema, Table, Column, Function, Procedure, TagAttachment), and their omission will likely cause issues when creating Atlas Query entities.
application_sdk/transformers/atlas/sql.py#L1239-L1284
application-sdk/application_sdk/transformers/atlas/sql.py
Lines 1239 to 1284 in 9f0b35b
Was this report helpful? Give feedback by reacting with 👍 or 👎
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelog
Additional context (e.g. screenshots, logs, links)
Checklist
Copyleft License Compliance