Made the Timeout in LicenseInformationService configurable via CLI argument (#584)

[kidcline1]

• Added new command line argument for generator (lto)
• Modified LicenseInformationService and LicenseInformationFetcher to allow passing timeout as an argument
• Removed hardcoded limit of 30 seconds

[kidcline1]

@microsoft-github-policy-service agree

[DaveTryon]

Strictly speaking, this is a breaking change because it changes a public interface; it would break any consumers of the API who had their own implementation of the ILicenseInformationFetcher interface. The non-breaking way to make this change is to leave ILicenseInformationFetcher unchanged, and to create a new interface:

public interface ILicenseInformationFetcher2 : ILicenseInformationFetcher
{
    /// <summary>
    /// Calls the ClearlyDefined API to get the license information for the list of components.
    /// </summary>
    /// <param name="listOfComponentsForApi"> A list of strings formatted into a list of strings that can be used to call the batch ClearlyDefined API.</param>
    /// <param name="timeoutInSeconds">Timeout in seconds to use when making web requests</param>
    /// <returns></returns>
    Task<List<string>> FetchLicenseInformationAsync(List<string> listOfComponentsForApi, int timeoutinSeconds);
}

Then you derive LicenseInformationFetcher from ILicenseInformationFetcher2. In ComponentDetectionBaseWalker.cs, you do something like this to safely call the method:

ILicenseInformationFetcher2 licenseInformationFetcher2 = licenseInformationFetcher as ILicenseInformationFetcher2;
// Maybe spit out a warning message if licenseInformationFetcher2 is null but configuration.LicenseInformationTimeout is not null
if (licenseInformationFetcher2 is null || configuration.LicenseInformationTimeout is null)
{
    apiResponses = await licenseInformationFetcher.FetchLicenseInformationAsync(listOfComponentsForApi);
}
else
{
    apiResponses = await licenseInformationFetcher2.FetchLicenseInformationAsync(listOfComponentsForApi, configuration.LicenseInformationTimeout.Value);
}

[DaveTryon]

Left several comments

[kidcline1]

Strictly speaking, this is a breaking change because it changes a public interface; it would break any consumers of the API who had their own implementation of the ILicenseInformationFetcher interface. The non-breaking way to make this change is to leave ILicenseInformationFetcher unchanged, and to create a new interface:

public interface ILicenseInformationFetcher2 : ILicenseInformationFetcher
{
    /// <summary>
    /// Calls the ClearlyDefined API to get the license information for the list of components.
    /// </summary>
    /// <param name="listOfComponentsForApi"> A list of strings formatted into a list of strings that can be used to call the batch ClearlyDefined API.</param>
    /// <param name="timeoutInSeconds">Timeout in seconds to use when making web requests</param>
    /// <returns></returns>
    Task<List<string>> FetchLicenseInformationAsync(List<string> listOfComponentsForApi, int timeoutinSeconds);
}

Then you derive LicenseInformationFetcher from ILicenseInformationFetcher2. In ComponentDetectionBaseWalker.cs, you do something like this to safely call the method:

ILicenseInformationFetcher2 licenseInformationFetcher2 = licenseInformationFetcher as ILicenseInformationFetcher2;
// Maybe spit out a warning message if licenseInformationFetcher2 is null but configuration.LicenseInformationTimeout is not null
if (licenseInformationFetcher2 is null || configuration.LicenseInformationTimeout is null)
{
    apiResponses = await licenseInformationFetcher.FetchLicenseInformationAsync(listOfComponentsForApi);
}
else
{
    apiResponses = await licenseInformationFetcher2.FetchLicenseInformationAsync(listOfComponentsForApi, configuration.LicenseInformationTimeout.Value);
}

I was wondering how best to handle that. Thank you for the suggestion - I have implemented the two new interfaces ILicenseInformationFetcher2 and ILicenseInformationService2 as you suggested to prevent breaking changes to the API.

[kidcline1]

Left several comments

Hello @DaveTryon, thank you for the thorough code review. I have made the requested changes in the latest commit.

[DaveTryon]

Thanks for the changes--it's very close. I think we need to be more explicit/restrictive in the allowed range of timeout values, make sure that the docs and tests cover the range, and we'll be ready to go

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 43.13725% with 29 lines in your changes missing coverage. Please review.

Project coverage is 69.98%. Comparing base (970a9e0) to head (66a6f16).

Files with missing lines	Patch %	Lines
...Sbom.Api/Executors/ComponentDetectionBaseWalker.cs	0.00%	12 Missing ⚠️
...ft.Sbom.Api/Executors/LicenseInformationFetcher.cs	0.00%	9 Missing ⚠️
...ft.Sbom.Api/Executors/LicenseInformationService.cs	0.00%	7 Missing ⚠️
src/Microsoft.Sbom.Api/Config/ConfigSanitizer.cs	94.44%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #773      +/-   ##
==========================================
- Coverage   70.13%   69.98%   -0.15%     
==========================================
  Files         277      277              
  Lines        8651     8700      +49     
  Branches     1006     1014       +8     
==========================================
+ Hits         6067     6089      +22     
- Misses       2065     2091      +26     
- Partials      519      520       +1     

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

[kidcline1]

Thanks for the changes--it's very close. I think we need to be more explicit/restrictive in the allowed range of timeout values, make sure that the docs and tests cover the range, and we'll be ready to go

Thanks. I've made the changes requested. For the change I commented on, I made the change based on what I think you were getting at since LicenseInformationFetcher and LicenseInformationService are so similar.

[EinmalIM]

I would like to set the timeout as well. Really looking forward to getting that feature.

[kidcline1]

Hello @DaveTryon, I've made the changes requested. Is there anything else I need to do on my end?

[DaveTryon]

Thanks, @kidcline1! I was busy on other tasks and lost track of this. These changes look really good to me. Assuming everything goes smoothly, we should get this merged in the next day or two. Our entire organization is currently operating under a release freeze for the holidays, so we won't be able to release the new version until early January.