Skip to content

Feature data sorting in route handler #7

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 4, 2025
Merged

Feature data sorting in route handler #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
805df37
feat: implement sortBy and sortOrder in data endpoint
fulleni Jul 4, 2025
a2cdb38
docs: update coverage badge and enhance key capabilities section
fulleni Jul 4, 2025
47d3b9c
docs: update coverage badge to reflect current percentage
fulleni Jul 4, 2025
bf35926
fix: correct coverage badge percentage formatting
fulleni Jul 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# ht_api

![coverage: percentage](https://img.shields.io/badge/coverage-XX-green)
![coverage: percentage](https://img.shields.io/badge/coverage-xx-green)
[![style: very good analysis](https://img.shields.io/badge/style-very_good_analysis-B22C89.svg)](https://pub.dev/packages/very_good_analysis)
[![License: PolyForm Free Trial](https://img.shields.io/badge/License-PolyForm%20Free%20Trial-blue)](https://polyformproject.org/licenses/free-trial/1.0.0)

Expand Down Expand Up @@ -32,6 +32,10 @@ management dashboard](https://github.com/headlines-toolkit/ht-dashboard).
including headlines, categories, and sources, through a well-structured
and protected API.

* 🔀 **Flexible Data Sorting:** Order lists of headlines, sources, and other
data by various fields in ascending or descending order, allowing for
dynamic and user-driven content presentation.

* 📊 **Dynamic Dashboard Summary:** Access real-time, aggregated metrics on
key data points like total headlines, categories, and sources, providing
an at-a-glance overview for administrative dashboards.
Expand Down
40 changes: 39 additions & 1 deletion routes/api/v1/data/index.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,34 @@ Future<Response> _handleGet(
final queryParams = context.request.uri.queryParameters;
final startAfterId = queryParams['startAfterId'];
final limitParam = queryParams['limit'];
final sortBy = queryParams['sortBy'];
Copy link

Choose a reason for hiding this comment

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

high

The sortBy query parameter is passed directly to the data layer without validation. This is a potential security and performance risk, as clients could request sorting on un-indexed or sensitive fields. You should validate the sortBy value against an allowlist of sortable fields for each model.

This validation should be performed within the switch (modelName) block, where the context of the model is known.

For example, for the headline model:

case 'headline':
  const allowedSortFields = {'createdAt', 'title'}; // Example allowlist
  if (sortBy != null && !allowedSortFields.contains(sortBy)) {
    throw BadRequestException(
      'Invalid "sortBy" parameter for model "headline". '
      'Allowed values are: ${allowedSortFields.join(', ')}.',
    );
  }
  allowedKeys = {'categories', 'sources', 'q'};
  // ... rest of the case

final sortOrderRaw = queryParams['sortOrder']?.toLowerCase();
final limit = limitParam != null ? int.tryParse(limitParam) : null;

SortOrder? sortOrder;
if (sortOrderRaw != null) {
if (sortOrderRaw == 'asc') {
sortOrder = SortOrder.asc;
} else if (sortOrderRaw == 'desc') {
sortOrder = SortOrder.desc;
} else {
throw const BadRequestException(
'Invalid "sortOrder" parameter. Must be "asc" or "desc".',
);
}
}
Comment on lines +116 to +127
Copy link

Choose a reason for hiding this comment

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

medium

The if-else if-else chain for parsing sortOrderRaw is functional, but a switch expression is more idiomatic and concise in modern Dart. Using a switch expression improves readability by clearly mapping input values to outcomes.

  SortOrder? sortOrder;
  if (sortOrderRaw != null) {
    sortOrder = switch (sortOrderRaw) {
      'asc' => SortOrder.asc,
      'desc' => SortOrder.desc,
      _ => throw const BadRequestException(
          'Invalid "sortOrder" parameter. Must be "asc" or "desc".',
        ),
    };
  }


final specificQueryForClient = <String, String>{};
final Set<String> allowedKeys;
final receivedKeys = queryParams.keys
.where((k) => k != 'model' && k != 'startAfterId' && k != 'limit')
.where(
(k) =>
k != 'model' &&
k != 'startAfterId' &&
k != 'limit' &&
k != 'sortBy' &&
k != 'sortOrder',
Comment on lines +133 to +138
Copy link

Choose a reason for hiding this comment

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

medium

The chain of && conditions to exclude standard query parameters is becoming long and can be hard to read. Using a Set and !contains is more concise and scalable if more parameters are added in the future. You can create the set inline for a succinct change.

        (k) => !{'model', 'startAfterId', 'limit', 'sortBy', 'sortOrder'}
            .contains(k),

)
.toSet();

switch (modelName) {
Expand Down Expand Up @@ -207,6 +229,8 @@ Future<Response> _handleGet(
userId: userIdForRepoCall,
startAfterId: startAfterId,
limit: limit,
sortBy: sortBy,
sortOrder: sortOrder,
);
Comment on lines 229 to 234
Copy link

Choose a reason for hiding this comment

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

medium

This switch statement contains significant code duplication. Every case block calls repo.readAllByQuery with the exact same set of arguments, differing only by the repository type. This makes the code harder to maintain, as any change to this call must be replicated in all case blocks.

While a full refactor might be beyond the scope of this PR, consider exploring ways to reduce this duplication in the future. One approach could be to enhance the ModelConfig to include a method that handles data retrieval, abstracting away the need for this large switch statement in the route handler.

case 'category':
final repo = context.read<HtDataRepository<Category>>();
Expand All @@ -215,6 +239,8 @@ Future<Response> _handleGet(
userId: userIdForRepoCall,
startAfterId: startAfterId,
limit: limit,
sortBy: sortBy,
sortOrder: sortOrder,
);
case 'source':
final repo = context.read<HtDataRepository<Source>>();
Expand All @@ -223,6 +249,8 @@ Future<Response> _handleGet(
userId: userIdForRepoCall,
startAfterId: startAfterId,
limit: limit,
sortBy: sortBy,
sortOrder: sortOrder,
);
case 'country':
final repo = context.read<HtDataRepository<Country>>();
Expand All @@ -231,6 +259,8 @@ Future<Response> _handleGet(
userId: userIdForRepoCall,
startAfterId: startAfterId,
limit: limit,
sortBy: sortBy,
sortOrder: sortOrder,
);
case 'user':
final repo = context.read<HtDataRepository<User>>();
Expand All @@ -239,6 +269,8 @@ Future<Response> _handleGet(
userId: userIdForRepoCall,
startAfterId: startAfterId,
limit: limit,
sortBy: sortBy,
sortOrder: sortOrder,
);
case 'user_app_settings':
final repo = context.read<HtDataRepository<UserAppSettings>>();
Expand All @@ -247,6 +279,8 @@ Future<Response> _handleGet(
userId: userIdForRepoCall,
startAfterId: startAfterId,
limit: limit,
sortBy: sortBy,
sortOrder: sortOrder,
);
case 'user_content_preferences':
final repo = context.read<HtDataRepository<UserContentPreferences>>();
Expand All @@ -255,6 +289,8 @@ Future<Response> _handleGet(
userId: userIdForRepoCall,
startAfterId: startAfterId,
limit: limit,
sortBy: sortBy,
sortOrder: sortOrder,
);
case 'app_config':
final repo = context.read<HtDataRepository<AppConfig>>();
Expand All @@ -263,6 +299,8 @@ Future<Response> _handleGet(
userId: userIdForRepoCall,
startAfterId: startAfterId,
limit: limit,
sortBy: sortBy,
sortOrder: sortOrder,
);
default:
throw OperationFailedException(
Expand Down
Loading