Skip to content

Conversation

@dkurepa
Copy link
Member

@dkurepa dkurepa commented Nov 26, 2025

@dkurepa dkurepa marked this pull request as ready for review November 26, 2025 13:30
Copilot AI review requested due to automatic review settings November 26, 2025 13:30
Copilot finished reviewing on behalf of dkurepa November 26, 2025 13:32
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 a new Namespace table to the Build Asset Registry (BAR) database to support multi-tenancy or environment isolation. The implementation introduces namespace associations for Channels, Subscriptions, DefaultChannels, and RepositoryBranches.

Key changes:

  • Creates a new Namespace database entity with unique name constraint
  • Adds nullable foreign key relationships from core entities (Channel, Subscription, DefaultChannel, RepositoryBranch) to Namespace
  • Configures environment-specific default namespaces ("production", "staging", "local") via appsettings
  • Updates API models and controllers to automatically assign the default namespace to newly created entities

Reviewed changes

Copilot reviewed 26 out of 32 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
src/Maestro/Maestro.Data/Models/Namespace.cs Defines new Namespace entity with Id and Name properties
src/Maestro/Maestro.Data/Models/Channel.cs Adds nullable Namespace navigation property
src/Maestro/Maestro.Data/Models/Subscription.cs Adds nullable Namespace navigation property
src/Maestro/Maestro.Data/Models/DefaultChannel.cs Adds nullable Namespace navigation property
src/Maestro/Maestro.Data/Models/Repository.cs Adds nullable Namespace navigation property for RepositoryBranch
src/Maestro/Maestro.Data/BuildAssetRegistryContext.cs Adds Namespaces DbSet and configures entity relationships with Restrict delete behavior; refactors connection string retrieval
src/Maestro/Maestro.Data/Migrations/20251125150730_AddNamespace.cs Database migration that creates Namespaces table, adds foreign keys, and seeds default namespace based on environment
src/Maestro/Maestro.Data/Migrations/20251125150730_AddNamespace.Designer.cs Auto-generated migration designer file reflecting the schema changes
src/Maestro/Maestro.Data/Migrations/BuildAssetRegistryContextModelSnapshot.cs Updates EF Core model snapshot to reflect new Namespace entity and relationships
src/ProductConstructionService/ProductConstructionService.Api/Api/EnvironmentNamespaceOptions.cs Configuration class for environment-specific default namespace name
src/ProductConstructionService/ProductConstructionService.Api/PcsStartup.cs Registers EnvironmentNamespaceOptions configuration
src/ProductConstructionService/ProductConstructionService.Api/appsettings.Development.json Configures "local" as default namespace for development
src/ProductConstructionService/ProductConstructionService.Api/appsettings.Staging.json Configures "staging" as default namespace for staging environment
src/ProductConstructionService/ProductConstructionService.Api/appsettings.Production.json Configures "production" as default namespace for production environment
src/ProductConstructionService/ProductConstructionService.Api/Api/v2020_02_20/Models/Namespace.cs API model for Namespace with read-only Id and Name properties
src/ProductConstructionService/ProductConstructionService.Api/Api/v2020_02_20/Models/Channel.cs Adds Namespace property to API model and updates constructor
src/ProductConstructionService/ProductConstructionService.Api/Api/v2020_02_20/Models/Subscription.cs Adds Namespace property to API model and updates constructor
src/ProductConstructionService/ProductConstructionService.Api/Api/v2020_02_20/Models/DefaultChannel.cs Adds Namespace property to API model and updates constructor
src/ProductConstructionService/ProductConstructionService.Api/Api/v2018_07_16/Models/RepositoryBranch.cs Adds Namespace property to API model and updates constructor
src/ProductConstructionService/ProductConstructionService.Api/Api/v2020_02_20/Controllers/ChannelsController.cs Injects EnvironmentNamespaceOptions and assigns default namespace on channel creation
src/ProductConstructionService/ProductConstructionService.Api/Api/v2020_02_20/Controllers/SubscriptionsController.cs Injects EnvironmentNamespaceOptions and assigns default namespace on subscription creation
src/ProductConstructionService/ProductConstructionService.Api/Api/v2020_02_20/Controllers/DefaultChannelsController.cs Injects EnvironmentNamespaceOptions and assigns default namespace on default channel creation
src/ProductConstructionService/ProductConstructionService.Api/Api/v2018_07_16/Controllers/RepositoryController.cs Injects EnvironmentNamespaceOptions and assigns default namespace on merge policy update
src/ProductConstructionService/Microsoft.DotNet.ProductConstructionService.Client/Generated/Models/Namespace.cs Generated client model for Namespace
src/ProductConstructionService/Microsoft.DotNet.ProductConstructionService.Client/Generated/Models/Channel.cs Adds Namespace property to generated client model
src/ProductConstructionService/Microsoft.DotNet.ProductConstructionService.Client/Generated/Models/Subscription.cs Adds Namespace property to generated client model
src/ProductConstructionService/Microsoft.DotNet.ProductConstructionService.Client/Generated/Models/DefaultChannel.cs Adds Namespace property to generated client model
src/ProductConstructionService/Microsoft.DotNet.ProductConstructionService.Client/Generated/Models/RepositoryBranch.cs Adds Namespace property to generated client model
test/ProductConstructionService.Api.Tests/TestDatabase.cs Seeds test namespace during test database initialization
test/ProductConstructionService.Api.Tests/ChannelsController20200220Tests.cs Configures test namespace options in dependency injection
test/ProductConstructionService.Api.Tests/SubscriptionsController20200220Tests.cs Configures test namespace options in dependency injection
test/ProductConstructionService.Api.Tests/DefaultChannelsController20200220Tests.cs Configures test namespace options in dependency injection
Files not reviewed (1)
  • src/Maestro/Maestro.Data/Migrations/20251125150730_AddNamespace.Designer.cs: Language not supported

Comment on lines 133 to 140
var defaultNamespace = await _context.Namespaces.SingleOrDefaultAsync(n => n.Name == _environmentNamespaceOptions.Value.DefaultNamespaceName);
defaultChannel = new Maestro.Data.Models.DefaultChannel
{
Channel = channel,
Repository = data.Repository,
Branch = data.Branch,
Enabled = data.Enabled ?? true
Enabled = data.Enabled ?? true,
Namespace = defaultNamespace
Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

Potential null reference issue: If SingleOrDefaultAsync returns null (no namespace found with the given name), this will assign null to the Namespace property. This should be handled consistently with error reporting if the namespace is expected to exist.

Consider adding validation:

var defaultNamespace = await _context.Namespaces.SingleOrDefaultAsync(n => n.Name == _environmentNamespaceOptions.Value.DefaultNamespaceName);
if (defaultNamespace == null)
{
    return StatusCode(500, new ApiError($"Default namespace '{_environmentNamespaceOptions.Value.DefaultNamespaceName}' not found."));
}
defaultChannel = new Maestro.Data.Models.DefaultChannel
{
    Channel = channel,
    Repository = data.Repository,
    Branch = data.Branch,
    Enabled = data.Enabled ?? true,
    Namespace = defaultNamespace
};

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

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

will make these Single and let it blow up if there's a null


public Namespace Namespace { get; }


Copy link

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

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

[nitpick] Extra blank line should be removed for consistency with the rest of the codebase.

Suggested change

Copilot uses AI. Check for mistakes.
Copy link
Member

@premun premun left a comment

Choose a reason for hiding this comment

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

@dkurepa you added the namespace into the response models of the API but I am not sure whether the namespace will be queried from the DB if you don't tell it to via Include():

image

Can you try if you get this from the API?

public static string GetConnectionString(string databaseName)
{
return $@"Data Source=localhost\SQLEXPRESS;Initial Catalog={databaseName};Integrated Security=true;Encrypt=false"; // CodeQL [SM03452] This 'connection string' is only for the local SQLExpress instance and has no credentials, Encrypt=false for .NET 8+ compatibility
var connectionString = $@"Data Source=localhost\SQLEXPRESS;Initial Catalog={databaseName};Integrated Security=true;Encrypt=false"; // CodeQL [SM03452] This 'connection string' is only for the local SQLExpress instance and has no credentials, Encrypt=false for .NET 8+ compatibility
Copy link
Member

Choose a reason for hiding this comment

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

Can this method be rather like this?

if (!string.IsNullOrEmpty(envVarConnectionString))
  connectionString = $@"..";
else
  connectionString = envVarConnectionString;

@@ -0,0 +1,1212 @@
// <auto-generated />
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this won't need the license header

Copy link
Member Author

Choose a reason for hiding this comment

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

looks like we don't have tit on any other Designer file. Also, VS not complaining about it, so I think it's not needed

@dkurepa
Copy link
Member Author

dkurepa commented Nov 26, 2025

@dkurepa you added the namespace into the response models of the API but I am not sure whether the namespace will be queried from the DB if you don't tell it to via Include():

image Can you try if you get this from the API?

It won't, no but I think that's ok. I don't think anyone is interested in the Namespace when they're looking for a subscription.
I did add it there to be kind of "complete" with the model.

@premun
Copy link
Member

premun commented Nov 26, 2025

I did add it there to be kind of "complete" with the model.

What do you mean complete? If it's never going to be populated, it's a bad model. The purpose of the model is not to have all the DTOs the same everywhere but to represent the data as close to reality as possible.

@dkurepa
Copy link
Member Author

dkurepa commented Nov 26, 2025

I did add it there to be kind of "complete" with the model.

What do you mean complete? If it's never going to be populated, it's a bad model. The purpose of the model is not to have all the DTOs the same everywhere but to represent the data as close to reality as possible.

fair, I'll remove it. When I do I think we won't need the client changes too

@dkurepa dkurepa merged commit 3bcbc4f into dotnet:main Nov 27, 2025
8 of 9 checks passed
@dkurepa dkurepa deleted the dkurepa/AddNamespaceToBAR branch November 27, 2025 07:34
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