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

Issue #1346 - Add ability to project document metadata to document properties #1364

Closed
wants to merge 2 commits into from

Conversation

barryhagan
Copy link
Contributor

These changes are somewhat involved in order to ensure that the projected metadata fields in the jsonb match the actual metadata columns as best as possible.

  • The metadata projector runs when saving documents in both unit of work and bulk insert scenarios.
  • The metadata projector also runs when the IIdentityMap maps a document on read. This should guarantee read-side consistency if you enable the feature on existing data.
  • The read-only metadata projected onto document properties is validated when saving and will throw if the metadata does not match what was loaded in the session.

In order to do the read-only validation, the metadata of a document is always stored in the IIdentityMap rather than making another db call. This also makes it possible to used cached metadata when calling MetadataFor<T> in an IQuerySession. It should only be required to go to the database for metadata if the doc wasn't already loaded in session (or loaded by a user defined query).

Performance considerations:

  • Additional metadata columns will be retrieved from the database for all queries to project and cache the DocumentMetadata
  • DocumentMetadata objects are stored in all implementations of IIdentityMap to perform read-only validations and support session based MetadataFor<T>.

Public API breaking changes:
The only potential breaking change is adding a parameter to Marten.Schema.Arguments.UpsertArgument.CompileBulkImporter() and its overrides. This seemed like something that should be internal anyway, but it could be adjusted if necessary to prevent a binary break on these public methods.

API surface change report:
https://github.com/barryhagan/marten/commit/236e1f312cf114556debaa3354e27b1cad484e3b/checks?check_suite_id=250920331#step:6:6

In a perfect world:

  • DocumentMetadata would have its own jsonb column in the db instead of being a combination of individual columns to make it easier to add arbitrary metadata going forward.

@oskardudycz
Copy link
Collaborator

Wow! @barryhagan I'll try to review it today or tomorrow.

@jeremydmiller
Copy link
Member

Hey all, I think I want to review this one. I'm a bit leery at how many changes this spawned.

@oskardudycz oskardudycz mentioned this pull request Oct 4, 2019
@mysticmind
Copy link
Member

@barryhagan sorry, a recent PR merge has resulted in a conflict with one of your file changes. Could you please rebase it?

@barryhagan
Copy link
Contributor Author

rebased

@mysticmind
Copy link
Member

@jeremydmiller could you please review this PR and provide your feedback?

@mysticmind
Copy link
Member

mysticmind commented Jan 30, 2020

@barryhagan, I was reviewing this and observed some breaking changes. Hence ran a binary compatibility check against v3.10.0 as baseline using ApiCompat and the results are as below:

Compat issues with assembly Marten:
InterfacesShouldHaveSameMembers : Interface member 'Marten.IQuerySession.MetadataFor<TDoc>(TDoc)' is present in the implementation but not in the contract.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Linq.Compiled.SkipOnCompiledQueryParsingAttribute' changed from '[AttributeUsageAttribute(64)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Method)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.DatabaseSchemaNameAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.DdlTemplateAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.DocumentAliasAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.DuplicateFieldAttribute' changed from '[AttributeUsageAttribute(384)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Field | AttributeTargets.Property)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.ForeignKeyAttribute' changed from '[AttributeUsageAttribute(384)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Field | AttributeTargets.Property)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.FullTextIndexAttribute' changed from '[AttributeUsageAttribute(388)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class | AttributeTargets.Field | AttributeTargets.Property)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.HiloSequenceAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.IdentityAttribute' changed from '[AttributeUsageAttribute(384)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Field | AttributeTargets.Property)]' in the implementation.
InterfacesShouldHaveSameMembers : Interface member 'Marten.Schema.IDocumentStorage.RegisterUpdate(System.String, Marten.Schema.UpdateStyle, Marten.Services.UpdateBatch, System.Object, System.Type)' is present in the implementation but not in the contract.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.IndexedLastModifiedAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.MultiTenantedAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.PropertySearchingAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.SoftDeletedAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.StructuralTypedAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.UniqueIndexAttribute' changed from '[AttributeUsageAttribute(384)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Field | AttributeTargets.Property)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.UseOptimisticConcurrencyAttribute' changed from '[AttributeUsageAttribute(4)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Class)]' in the implementation.
CannotChangeAttribute : Attribute 'System.AttributeUsageAttribute' on 'Marten.Schema.VersionAttribute' changed from '[AttributeUsageAttribute(384)]' in the contract to '[AttributeUsageAttribute(AttributeTargets.Field | AttributeTargets.Property)]' in the implementation.
MembersMustExist : Member 'Marten.Schema.Arguments.CurrentVersionArgument.CompileBulkImporter(Marten.Schema.DocumentMapping, Marten.EnumStorage, System.Linq.Expressions.Expression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'Marten.Schema.Arguments.DocJsonBodyArgument.CompileBulkImporter(Marten.Schema.DocumentMapping, Marten.EnumStorage, System.Linq.Expressions.Expression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'Marten.Schema.Arguments.DocTypeArgument.CompileBulkImporter(Marten.Schema.DocumentMapping, Marten.EnumStorage, System.Linq.Expressions.Expression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'Marten.Schema.Arguments.DotNetTypeArgument.CompileBulkImporter(Marten.Schema.DocumentMapping, Marten.EnumStorage, System.Linq.Expressions.Expression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'Marten.Schema.Arguments.TenantIdArgument.CompileBulkImporter(Marten.Schema.DocumentMapping, Marten.EnumStorage, System.Linq.Expressions.Expression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'Marten.Schema.Arguments.UpsertArgument.CompileBulkImporter(Marten.Schema.DocumentMapping, Marten.EnumStorage, System.Linq.Expressions.Expression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'Marten.Schema.Arguments.VersionArgument.CompileBulkImporter(Marten.Schema.DocumentMapping, Marten.EnumStorage, System.Linq.Expressions.Expression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression, System.Linq.Expressions.ParameterExpression)' does not exist in the implementation but it does exist in the contract.
InterfacesShouldHaveSameMembers : Interface member 'Marten.Services.IIdentityMap.MetadataCache' is present in the implementation but not in the contract.
InterfacesShouldHaveSameMembers : Interface member 'Marten.Services.IIdentityMap.Get<T>(System.Object, System.Type, System.IO.TextReader, System.Nullable<System.Guid>, System.Func<T, Marten.Storage.DocumentMetadata>)' is present in the implementation but not in the contract.
InterfacesShouldHaveSameMembers : Interface member 'Marten.Services.IIdentityMap.MetadataCache.get()' is present in the implementation but not in the contract.
Total Issues: 29

Note that some of them are false positives i.e. CannotChangeAttribute rule.

Could you please check on the above?

@mysticmind
Copy link
Member

mysticmind commented Jan 30, 2020

@jeremydmiller @oskardudycz @jokokko I will send a PR later today (already implemented it) which will help us run the binary compatibility checks using ApiCompat, see details here

@jeremydmiller
Copy link
Member

I'm merging this into #1337

@jeremydmiller
Copy link
Member

I shouldn't have closed this yesterday:(

What's going to happen though is that I'm probably going to take the configuration code and the tests from this pull request, but use a different implementation in the v4 codebase

@jeremydmiller jeremydmiller reopened this Sep 19, 2020
@dnfadmin
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ barryhagan sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

@jeremydmiller
Copy link
Member

I cherry picked this a little bit and used the configuration and tests from this PR to make the real feature in v4.

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.

5 participants