-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Soft Delete enhancements #139 #2807
Conversation
if (!IsEntity(entity.GetType())) | ||
{ | ||
throw new AbpException(entity.GetType() + " is not an Entity !"); | ||
} | ||
|
||
return ReflectionHelper.GetValueByPath(entity, entity.GetType(), "Id"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi
entity
that implement the IEntity
interface do not have an Id
property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello maliming,
So you mean if an entity is inherited from Entity
(or IEntity
) it won't pass this check which will be causing problems. Do I get it right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Entity will pass the check, but it does not have an Id
property. Only entities that inherit the IEntity<IdKeyType>
interface will have an Id
property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok got it now..
So it should be something like;
if (!ReflectionHelper.IsAssignableToGenericType(entity.GetType(), typeof(IEntity<>)))
{
throw new AbpException(entity.GetType() + " is not an Entity with an Id property !");
}
Seems good to go by this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the GetKeys
method of entity can be used. This way we can delete the entities that implement IEntity
and IEntity<T>
.
public abstract object[] GetKeys(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if it is viable. Do we want to hard delete only IEntity
implemented entities?
If an entity has no Id
property, it can be many-to-many entity without Id property (juntion table) which we wouldn't want to hard delete them all right?
I may have lack of knowledge about use cases of Entity
and IEntity
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some documents we can refer to.
https://docs.abp.io/en/abp/latest/Entities#entities-with-composite-keys
{ | ||
repository.HardDelete(entity); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi
Repositories are no longer providing synchronization methods.
#2026 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah makes sense. Will remove sync methods in next commit.
I merged by making some improvements and refactoring: fc8d34e |
Resolves #139
Added
HardDelete
andHardDeleteAsync
methods toRepositoryExtensions
.Added HardDelete Tests for EfcoreRepository and MongoDbRepository.