Skip to content

Conversation

JIAQIA
Copy link
Contributor

@JIAQIA JIAQIA commented Jul 25, 2025

Description:
This PR fixes a TypeError in the async smartquery() method where HTTP method references (POST/GET) were incorrectly being awaited during assignment rather than during execution.

Changes:

  1. Removed incorrect await when assigning self.client.POST/GET method references
  2. Added proper await when actually making the request
  3. Applied same correction to the special case handling for Elasticsearch/Testdata

Impact:
• Fixes broken POST request functionality in async datasource queries

• Maintains consistent async behavior throughout the method

• Follows proper async/await patterns for HTTP requests

Testing:
Manual verification was performed by:

  1. Executing POST queries against test datasources
  2. Verifying both direct queries and expression-based queries
  3. Checking special case handling paths

Related Issues:
#223

Checklist:
Code follows the project's style guidelines

Tests pass (or N/A - this bug wasn't caught by existing tests)

@JIAQIA JIAQIA requested a review from amotl as a code owner July 25, 2025 06:54
Copy link

codecov bot commented Jul 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.94%. Comparing base (7acc4d8) to head (dad78c2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #224      +/-   ##
==========================================
- Coverage   92.40%   89.94%   -2.47%     
==========================================
  Files          27       27              
  Lines        1818     1810       -8     
==========================================
- Hits         1680     1628      -52     
- Misses        138      182      +44     
Flag Coverage Δ
unittests 89.94% <ø> (-2.47%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@amotl
Copy link
Contributor

amotl commented Jul 25, 2025

Hi. Thanks for your patch. Maybe this is something to consider for the async code generator instead?

/cc @Ousret

@JIAQIA
Copy link
Contributor Author

JIAQIA commented Jul 25, 2025

Perhaps, but the current code is indeed not working properly. I'm using the smartquery interface, so fixing this issue would be really helpful for me. Otherwise, I'd have to use synchronous methods for this interface, which would be quite cumbersome.

@amotl
Copy link
Contributor

amotl commented Jul 25, 2025

We hear you. The thing is that the async code is getting generated, so I think we need to fix the generator instead. Maybe you can also make a difference there? generate_async.py it is.

@amotl
Copy link
Contributor

amotl commented Jul 26, 2025

I have to admit I am looking at the very ingredients of generate_async.py for the first time in my life. Apperently, it is not a generic translator, but tailored to the codebase instead.

I think the culprit is around this spot at line 104.

module_dump = re.sub(r"( {4}def )(?!_)", r" async def ", module_dump)
module_dump = module_dump.replace("self.client.", "await self.client.")
for relative_import in [".base", "..client", "..knowledge", "..model"]:
module_dump = module_dump.replace(f"from {relative_import}", f"from .{relative_import}")
module_dump = module_dump.replace("self.api.version", "await self.api.version")
module_dump = module_dump.replace("= self.", "= await self.")

JIAQIA and others added 2 commits July 26, 2025 22:53
This fixes a TypeError that occurred when trying to await HTTP method references
instead of properly calling them. The changes:

1. Remove incorrect `await` when assigning POST/GET method references
2. Add proper `await` when actually making the request

The bug prevented successful POST requests in the async datasource query functionality.
@amotl
Copy link
Contributor

amotl commented Jul 26, 2025

Hi again. I've just added the fix for the code converter to complete the job. Thanks again!

@amotl amotl requested a review from Ousret July 26, 2025 21:01
@amotl amotl merged commit 2a8c256 into grafana-toolbox:main Jul 26, 2025
9 checks passed
@Ousret
Copy link
Collaborator

Ousret commented Aug 1, 2025

Great catch! I maybe available soon to implement unasync and mirror all the tests also.

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