-
Notifications
You must be signed in to change notification settings - Fork 44
Add support for used in endpoints #403
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add null handling and consider changing EnumerateFeed to async
| /// <param name="codename">The codename of a content item.</param> | ||
| /// <param name="parameters">A collection of query parameters for filtering.</param> | ||
| /// <returns>The <see cref="DeliveryUsedInItems"/> instance that can be used to enumerate through content item parents for the specified item codename. If no query parameters are specified, default language parents are enumerated.</returns> | ||
| public IDeliveryItemsFeed<IUsedInItem> GetItemUsedIn(string codename, IEnumerable<IQueryParameter> parameters = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
existing methods accepting a codename parameter have null handling in place. consider adding it to all usedIn methods as well:
if (codename == null)
{
throw new ArgumentNullException(nameof(codename), "The codename of a content item is not specified.");
}| /// <returns>The <see cref="DeliveryUsedInItems"/> instance that can be used to enumerate through asset parents for the specified asset codename. If no query parameters are specified, default language parents are enumerated.</returns> | ||
| public IDeliveryItemsFeed<IUsedInItem> GetAssetUsedIn(string codename, IEnumerable<IQueryParameter> parameters = null) | ||
| { | ||
| ValidateUsedInParameters(parameters); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also null handling:
if (codename == null)
{
throw new ArgumentNullException(nameof(codename), "The codename of an asset is not specified.");
}| #endregion | ||
| #region "Private methods" | ||
| private static IEnumerable<T> EnumerateFeed<T>(IDeliveryItemsFeed<T> feed) where T : class | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method is synchronous but invokes an async method and waits for its result, effectively blocking threads. not sure if this is a valid scenario under the circumstances, but I'm not familiar with Rx so I asked AI about it.
the fix is simple, make the method async and have it return IAsyncEnumerable:
private static async IAsyncEnumerable<T> EnumerateFeed<T>(IDeliveryItemsFeed<T> feed) where T : class
{
while (feed.HasMoreResults)
{
var batch = await feed.FetchNextBatchAsync();
foreach (var contentItem in batch.Items)
{
yield return contentItem;
}
}
}however, related methods would need to be updated as well.
again, I'm not sure if thread locking or even deadlocks can be a thing in our scenario. also, Rx is used much less, compared to async, so I leave this at your decision.
Motivation
Add support for new used-in delivery API feature.
Checklist
Release date
Please do not release before the used-in is released in the production. The early access should be released in the middle of March, we will let you know.