Skip to content
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

Annotations not preserved with adjustindexes [NPGSQL] #829

Open
fbjerggaard opened this issue May 21, 2024 · 2 comments · May be fixed by #832
Open

Annotations not preserved with adjustindexes [NPGSQL] #829

fbjerggaard opened this issue May 21, 2024 · 2 comments · May be fixed by #832

Comments

@fbjerggaard
Copy link

Using Finbuckle.MultiTenant 6.13.1 - Haven't tried in 7.0.1 but a quick look at the source tells me there are no changes in this part of the source.

Have the following code:

NpgsqlIndexBuilderExtensions.IncludeProperties(
	builder.HasIndex(x => new { x.Street, x.CountryCode, x.ZipCode, x.Created }),
	x => new { x.City, x.Coordinates, x.ValidatedBy })
	.IsCreatedConcurrently();
builder.IsMultiTenant().AdjustIndexes();

This results in the following migration (when creating only this index):

migrationBuilder.CreateIndex(
	name: "ix_address_street_country_code_zip_code_created",
	table: "address",
	columns: new[] { "street", "country_code", "zip_code", "created" })

However, swapping those two lines in the configuration "fixes" it:

builder.IsMultiTenant().AdjustIndexes();

NpgsqlIndexBuilderExtensions.IncludeProperties(
	builder.HasIndex(x => new { x.Street, x.CountryCode, x.ZipCode, x.Created }),
	x => new { x.City, x.Coordinates, x.ValidatedBy })
	.IsCreatedConcurrently();

Results in the following migration:

migrationBuilder.CreateIndex(
	name: "ix_address_street_country_code_zip_code_created",
	table: "address",
	columns: new[] { "street", "country_code", "zip_code", "created" })
	.Annotation("Npgsql:CreatedConcurrently", true)
	.Annotation("Npgsql:IndexInclude", new[] { "city", "coordinates", "validated_by" });

The only part missing is the TenantID here, which is expected due to the order in the configuration.

I think it might be fixable by adding this to the AdjustIndex method in MultiTenantEntityTypeBuilder

if (index.GetAnnotation() is IIndexAnnotation annotation)
{
	indexBuilder.HasAnnotation(annotation);
}
@AndrewTriesToCode
Copy link
Sponsor Contributor

I agree. Would you like to submit a PR or wait for me to make the change? Also in the first set of results you share above shouldn't tenant id be part of the generated migration?

@fbjerggaard
Copy link
Author

Also in the first set of results you share above shouldn't tenant id be part of the generated migration?

Yes it should - I manually edited the code when pasting into here and forgot about that.

I can take a look at implementing the fix on monday

fbjerggaard pushed a commit to fbjerggaard/Finbuckle.MultiTenant that referenced this issue May 26, 2024
fbjerggaard added a commit to fbjerggaard/Finbuckle.MultiTenant that referenced this issue May 26, 2024
fbjerggaard pushed a commit to fbjerggaard/Finbuckle.MultiTenant that referenced this issue Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants