Skip to content

Conversation

@JBrenesSimpat
Copy link
Contributor

No description provided.

JBrenesSimpat and others added 9 commits November 14, 2025 14:34
* POST ApiClient

* Test coverage

* Add Fields

* Fix tests

* Fix tests

* Update ApiClient and Tests

* Delete ApiClient

* Reset credential functionality

* Fix transaction scope

* Rollback IDP Repository changes on failure
* Kafka test setup in Instance Management E2E

* Build errors

* Todos
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds comprehensive support for year-based routing via a dropdown selector in Swagger UI, along with extensive Kafka topic-per-instance testing infrastructure and API client management endpoints.

Key Changes:

  • Added school year dropdown to Swagger UI that dynamically extracts available years from OpenAPI specs and automatically selects the current year based on date
  • Implemented comprehensive Kafka topic-per-instance testing infrastructure with message collection, isolation validation, and E2E test scenarios
  • Added full CRUD operations for API clients with identity provider synchronization and credential management

Reviewed Changes

Copilot reviewed 41 out of 41 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
MetadataEndpointModule.cs Generates server URLs with school year variables from DMS instances for OpenAPI specs
edfi-school-year-from-spec.js Swagger UI plugin that extracts years from specs and creates year selector dropdown
swagger-initializer.js Adds request interceptor to inject selected year into API calls
InstanceKafkaStepDefinitions.cs E2E test step definitions for Kafka message validation and topic isolation
ApiClientModule.cs Implements POST/PUT/DELETE/reset-credential endpoints with identity provider coordination
Dms-Management.psm1 PowerShell functions for creating school year instances with route contexts
setup-instance-kafka-connectors.ps1 Automated Debezium connector setup for topic-per-instance architecture
ApiClient*.cs (datamodel) Command/response models for API client operations
ApiClientRepository.cs Database operations for API client CRUD with DMS instance mappings

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +22 to +33
private static JsonArray GetServers(HttpContext httpContext, IDmsInstanceProvider dmsInstanceProvider)
{
return new JsonArray { new JsonObject { ["url"] = $"{httpContext.Request.RootUrl()}/data" } };
// Get all school years from DMS instances
var instances = dmsInstanceProvider.GetAll();
var schoolYears = instances
.SelectMany(instance => instance.RouteContext)
.Where(kvp => kvp.Key.Value.Equals("schoolYear", StringComparison.OrdinalIgnoreCase))
.Select(kvp => kvp.Value.Value)
.Distinct()
.OrderByDescending(year => year)
.ToList();

Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

[nitpick] The schoolYears collection is computed every time GetServers is called, but the instances rarely change. Consider caching this collection with cache invalidation when instances are modified to avoid repeated computation on every OpenAPI spec request.

Suggested change
private static JsonArray GetServers(HttpContext httpContext, IDmsInstanceProvider dmsInstanceProvider)
{
return new JsonArray { new JsonObject { ["url"] = $"{httpContext.Request.RootUrl()}/data" } };
// Get all school years from DMS instances
var instances = dmsInstanceProvider.GetAll();
var schoolYears = instances
.SelectMany(instance => instance.RouteContext)
.Where(kvp => kvp.Key.Value.Equals("schoolYear", StringComparison.OrdinalIgnoreCase))
.Select(kvp => kvp.Value.Value)
.Distinct()
.OrderByDescending(year => year)
.ToList();
// Static cache for school years and last instance hash
private static List<string>? _cachedSchoolYears = null;
private static int _cachedInstanceHash = 0;
private static readonly object _schoolYearsCacheLock = new object();
private static JsonArray GetServers(HttpContext httpContext, IDmsInstanceProvider dmsInstanceProvider)
{
// Get all school years from DMS instances, with caching
var instances = dmsInstanceProvider.GetAll();
// Compute a hash of the instances to detect changes
int instanceHash = instances.Count;
foreach (var instance in instances)
{
instanceHash = HashCode.Combine(instanceHash, instance.GetHashCode());
}
List<string> schoolYears;
lock (_schoolYearsCacheLock)
{
if (_cachedSchoolYears == null || _cachedInstanceHash != instanceHash)
{
schoolYears = instances
.SelectMany(instance => instance.RouteContext)
.Where(kvp => kvp.Key.Value.Equals("schoolYear", StringComparison.OrdinalIgnoreCase))
.Select(kvp => kvp.Value.Value)
.Distinct()
.OrderByDescending(year => year)
.ToList();
_cachedSchoolYears = schoolYears;
_cachedInstanceHash = instanceHash;
}
else
{
schoolYears = _cachedSchoolYears;
}
}

Copilot uses AI. Check for mistakes.

var clientId = Guid.NewGuid().ToString();
var clientSecret = RandomNumberGenerator.GetString(
"0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ",
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

[nitpick] The character set for secret generation excludes special characters. For improved security, consider including special characters to increase entropy and password strength. The current 62-character set provides ~190 bits of entropy for 32 characters, but adding special characters would increase this further.

Suggested change
"0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ",
"0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!@#$%^&*()-_=+[]{}|;:,.<>/?",

Copilot uses AI. Check for mistakes.
{
await identityProviderRepository.CreateClientAsync(
apiClient.ClientId,
"ROLLBACK_PLACEHOLDER_SECRET", // Cannot recover original secret
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

Using a placeholder secret during rollback creates a security vulnerability where the recreated client has a known, hardcoded credential. Consider either preventing deletion entirely if rollback fails, or generating a new random secret and logging it for manual recovery.

Copilot uses AI. Check for mistakes.
{
BootstrapServers = _configuration.BootstrapServers,
GroupId = _configuration.ConsumerGroupId,
AutoOffsetReset = AutoOffsetReset.Latest, // Only collect messages after we start
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

[nitpick] Using AutoOffsetReset.Latest means messages published before the consumer starts will be missed. While this may be intentional for testing, consider documenting this behavior more prominently in the class documentation to ensure test authors understand timing dependencies.

Copilot uses AI. Check for mistakes.
Comment on lines +114 to +115
const currentMonth = currentDate.getMonth() + 1; // JavaScript months are 0-indexed
const calculatedCurrentYear = currentMonth > 6 ? currentCalendarYear + 1 : currentCalendarYear;
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

[nitpick] The school year calculation logic (July 1 transition) is hardcoded. Consider making the transition month configurable or documenting this assumption more prominently, as different educational systems may use different school year boundaries.

Copilot uses AI. Check for mistakes.
Comment on lines +30 to +31
RuleForEach(a => a.EducationOrganizationIds).GreaterThan(0);
RuleForEach(a => a.DmsInstanceIds).GreaterThan(0);
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

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

The NotNull() validation was removed from RuleForEach, but primitive longs cannot be null. However, if EducationOrganizationIds or DmsInstanceIds collections contain nullable longs (long?), this could allow null values through. Verify that the collections are defined as non-nullable long arrays.

Copilot uses AI. Check for mistakes.
@JBrenesSimpat JBrenesSimpat deleted the DMS-865 branch November 20, 2025 15:04
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