Skip to content

Conversation

@LordMayonyies
Copy link
Contributor

Overview

  • Introduce a reusable ApiReader<TResult> that centralises auth/header/retry logic and returns strongly typed responses.
  • Retool the existing ApiReader to inherit from the generic base so DataRow pagination + JSON parsing still work.
  • Extend PipeFlow.From.Api with typed and async entry points, and add a matching unit-test suite covering the new reader’s behaviours.

Key Changes

  • PipeFlow/Api/ApiReaderGeneric.cs: new generic reader with typed Read/ReadAsync, HTTP retry logic, auth/header configuration, and IDisposable support.
  • PipeFlow/Api/ApiReader.cs: now sealed and derived from the generic reader; keeps pagination, JSON parsing into DataRow, and overrides FetchDataWithRetry.
  • PipeFlow/PipeFlow.cs: adds Api<TResult> and ApiAsync<TResult> helpers so pipelines can consume strongly typed DTOs or await the async fetch path.
  • PipeFlow.Tests/ApiReaderGenericTests.cs: covers deserialization, sync wrapper, auth/headers, retry success/failure/default paths, header overwrites, and disposal – ensuring the generic reader is fully exercised.

Why It Matters

  • Consumers can hydrate custom DTOs directly without post-processing DataRow dictionaries.
  • Shared HTTP concerns (auth, headers, retries) live in one place, simplifying future features.
  • Async pipeline support avoids synchronous blocking when fetching from APIs.
  • Dedicated tests provide confidence that the generic reader handles happy-path and failure scenarios.

@Nonanti
Copy link
Owner

Nonanti commented Sep 20, 2025

look's good

@Nonanti
Copy link
Owner

Nonanti commented Sep 20, 2025

Code Review - Potential Deadlock Concerns

Thanks for the PR! The generic ApiReader implementation looks great overall. However, I've identified some potential deadlock risks that should be addressed
before merging:

Critical Issue: Sync-over-Async Pattern

The Read() method appears to be blocking on an async operation. If it uses .Result, .Wait(), or .GetAwaiter().GetResult() to call ReadAsync(), this will cause deadlocks
in environments with a SynchronizationContext (ASP.NET, WPF, WinForms).

Missing ConfigureAwait(false)

The async methods should use ConfigureAwait(false) to avoid capturing the synchronization context unnecessarily. This is especially important for library code.

Recommendations:

  1. Either remove the synchronous Read() method entirely or implement it in a deadlock-safe manner
  2. Add ConfigureAwait(false) to all await calls in the library code
  3. Document any limitations if the sync method is kept

These changes will prevent potential deadlocks and make the library safer to use across different application types.

@LordMayonyies
Copy link
Contributor Author

LordMayonyies commented Sep 22, 2025

I applied the Code Review recommendations along with other minor improvements. As soon as you review it you can merge it.

Thanks for starting the project, it helped me a lot simplifying some ETL programs I have.

Also, when will you release the next version?

@Nonanti Nonanti merged commit bab1bcf into Nonanti:main Sep 23, 2025
6 checks passed
@LordMayonyies LordMayonyies deleted the AddApiReaderWIthGenericType branch September 23, 2025 15:15
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.

2 participants