Review: Bug fix#17149 User Validation on Audit Repository .Save Method

[IbrahimMNada]

Description

If there's an existing issue for this PR then this fixes #17149

this pull request is a fix for this issue : #17149

Which states that a content publish can be established using ContentService with a non existing user.

A Simple Database Validation/ and simple caching mechanism were added in order to check if the user really exists in the database

---

This item has been added to our backlog AB#46955

[github-actions]

Hi there @IbrahimMNada, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[JasonElkin]

Hey @IbrahimMNada, thanks for making a PR to fix this 🎉

Someone from the team will get this reviewed and get back to you soon.

[IbrahimMNada]

Any update on this
😅

[mikecp]

Hello @IbrahimMNada,

Apologies for the delayed answer 😅
Your update looks fine, but I was wondering if you had given a try to @Zeegaan 's suggestion to use the IUserService to get the User info?
I'm asking because that would probably make a more robust solution, and since the class is internal, I don't think it would be a problem to inject the service in the constructor for example.

So it would be super great if you could check how that would work out😁

Cheers!

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 suggestion.

Comments skipped due to low confidence (1)

src/Umbraco.Infrastructure/Persistence/Repositories/Implement/AuditRepository.cs:197

• [nitpick] The error message 'The user id does not exist' could be more descriptive. Suggestion: 'The specified user ID does not exist in the database.'

new { id = entity.UserId }) ?? throw new InvalidOperationException("The user id does not exist");

[IbrahimMNada]

I have no problem injecting the IUserService in the Audit repository ,
but wont that break the paradigm of the idea that repositories should not depend on services ?
its the other way around , services should depend on repositories.
and it would cause a circular dependency leading to a stack-overflow if we later needed to inject the repos into a service.
(please correct if I'm wrong) I have randomly checked some repositories didnt seen any ones that uses services.

plus if we go with services we lose the caching , which in my experience is gonna be useful since most of the time me or the people I know used content services mostly for migration/bulk transitions.

but never the less its you call , i'm ready to change it

[mikecp]

Woopsie my bad, I did not pay attention to the fact that it was a Repository 😅

Then it's indeed fine 👍

[mikecp]

Hi again @IbrahimMNada ,

I took, a further look, it all works fine, thanks a lot 👍

Before merging, I wanted to suggest trying to get rid of the hardcoded SQL statement, which can turn out tricky to maintain (aka we forgot about them😅 ) through time and maybe try to take inspiration from what is done in the PerformGet method a little above.

This could give like the code below (not tested), which I think would be easier to maintain if some DB related things change through time.

    Sql<ISqlContext> sql = SqlContext.Sql();
    sql = sql.Selectt<UserDto>();
    sql.Where(GetBaseWhereClause(), new { Id = entity.UserId });
   
    int? userIdQueryResult = Database.FirstOrDefault<UserDto>(sql) ?? throw new InvalidOperationException("The user id does not exist");

Would you maybe be up for trying this approach 😁?

No problem if you don't have the time to, then I'll merge your code as-is 👍

Cheers!

[IbrahimMNada]

Not a Problem
I will change/try today for sure

[IbrahimMNada]

Done , I have changed it to follow Npoco annotations , made the method more reusable/overridable for other consumers

manually tested it , working as expected

[Zeegaan]

Hello 👋
I just had a talk with the team about this proposed solution, and although we really appreciate the time you've taken to work on this, we don't think that this s the solution we want.

We want to fix this in the AuditService itself, and it should return and Attempt, and if the user doesn't exist, we will return a failed attempt, this matches the other services patterns 👍
Altough this means refactoring a lot of places, where services use the AuditRepository directly, it will be much easier to maintain going forward.

We will mark the issue as a sprint candidate, so this works goes in our backlog.