IMO it should in fact return 404 as this is semantically more relevant than the other options. Its not a bad request because the request is valid but refers to a nonexistent resource (parent). If it was a db query it would return an empty list due to the WHERE parent_id clause not referring to existing parent. But then SQL query does not have the ability to return semantic results. An empty list is ambiguous between no child entries or parent not found. If the route is .../customer/4/orders , seeing a 404 can only really mean one thing. Customer 4 not found. You can always provide more info in the 404 in the extended error properties of the response
-
Join the MyBroadband community
South Africa’s biggest forum. Discuss, discover, and connect with thousands of members.
-
Question of the Day: What is the ultimate French toast topping?
Looking for urgent help with API
- Thread starter Solarion
- Start date
Yeah. Definitely can get fully on board with this reasoning.
What are your thoughts on the function “getOrdersByCustomerId”?
You mean as an enpoint? I didnt read the thread though..
edit: not sure what u asking?
No, I mean as a stand alone function. C# or whatever language you are using.
A developer would typically use this signature
Code:
List<Order> GetOrdersByCustomerId(int customerId)Passing an customer id that does not exist, would you return: empty list, null, error/throw exception, other?
Should this function even exist?
Last edited:
Ah ok. typing a bit tedious on mobile but here goes.
a method like that tends to assume an impedance match with a db but that may well be a code smell. Not sure. But what if it is an interface that is agnostic to the storage layer. From a semantically significant point of view it could return null if customer not found whether from webservice or db. Empty list when customer exists but no items. with a webservice a 404 could result in the null being returned. Or empty list For no items. Easy peasy because the web response tells you what happened.
not so with a sql query. You will get empty list in either case.
one approach i have followed is to change the signature to take Customer instead of long customerId. The advantage is that the Customer argument could assume the argument represents a fully hydrated customer object, implying it exists. This is also semantically more sound. The downside is you have to have queried / GET the customer prior to this.
I think because most of us think relationally the signature as you have it is our first stab. If you start to think in terms of domain and aggregate roots, my suggested approach may make sense. You first query the root and then lazy load the children when required. Whether from db or webapi.
but TBH not sure what the ‘right’ way is. Likely be pragmatic and horses for courses.
Last edited:
100%
I actually made an edit while you were replying alluding to exactly this: should this function even exist?
If I WAS going to use a function with that signature, i would slightly change it to possible convey more meaning (this meaning however is probably conveyed more by team knowledge/style guides rather than the signature itself - flipping difficult to have these discussions in text on a mobile
)What we do is use Option/Maybe
Code:
Option<List<Order>> GetOrdersByCustomerId(int customerId)You’ll maybe a get a list back. Obviously the “why you aren’t getting a list back” is the missing part here, but we can agree as a team that it means X, and understand that because of the convention established.
My goal is for less opaque code. I think that passing “already hydrated parents” does this.
100% sold/converted on the 404 btw
DA-LION-619
Honorary Master
No it isn’t, you’re asking for orders not the customer.
Empty list or not, the customer could be deleted/deactivated.
If you’re modelling based on SQL,
…/orders?customerId=4
In the beginning sure, but this is why I like Jimmy’s feature approach.
Add multiple sort orders and things start getting messy, if it breaks then it breaks a lot of things everywhere.
If a feature breaks, it’s just that feature.
Last edited:
Kosmik
Honorary Master
Interesting but why would AsNoTracking make a difference in this scenario? No changes are being done to the Rental object so the model wasn't updated and hence the db should not once changes are sent?
Kosmik
Honorary Master
The endless 404 debate.Technically, as per MS standard, 404 is the correct response but the the conflict comes in that 404 is seen often as a web traffic failure ie: webpage or api NOT found vs resource not found. One could return a 204, which indicates a successful call was made but no data was found. However, some security concerns are that one should not do that as it supposedly exposes the means for someone to hammer the call with "probing" data.
So one concept I found over the years is that if its data related, based on design, one can use 404 or 204 depending on the exposure of the endpoint ( public facing or internal ). A LOT of public web devs, hated the fact of the 404 as they saw it as a error and the webstandard does stipulate it is a "client error". So in those cases, 204 worked fine but that often also required education. If it was internal, 404 was fine as the internal teams were aware of it.
If the api was security related, 404 was deemed the norm as you do not want to expose security validation pass/failures.
Some reference.
vs
Lots of web devs see 404 and don't look deeper at the suggested included message.
battletoad
Expert Member
- Joined
- Mar 10, 2009
- Messages
- 1,461
- Reaction score
- 52
AsNoTracking skips adding the entity to EF's change tracker. An implication of this is that whenever these untracked entities are added as navigations for other entities, and those other entities are saved/updated, EF will perform an insert for the untracked entity (violating PK/unique constraints etc. in best case scenarios). That insert attempt will happen even if the untracked entity is unchanged.
I'd argue you can't blame EF for this behaviour though as it wouldn't know whether the untracked entity originally came from the database due to AsNoTracking, and so to keep things consistent it will attempt to do an insert.
Note, you can attach it/muck about with the entity's state to fix up the tracking graph before performing persistence operations, but then you'll need to expose additional methods on the repository (I doubt anyone would want to import the context outside of the persistence layer).
battletoad
Expert Member
- Joined
- Mar 10, 2009
- Messages
- 1,461
- Reaction score
- 52
you could end up with an endpoint /customers/4/orders/123/states, with timestamp and states like paid, rentalOccupied, checkedOut, etc. 404's in this case could imply either customerId 4 - , order 123 - or both - do(es) not exist.
More importantly, does it matter which parent doesn't exist? Or put differently, what benefit is derived from knowing which parent resource does not exist (apart from performing a couple of GETs starting at the root resource and moving outwards)?
get out of my head - was thinking about that exact thing while trying to fall asleep
IMO, in that case, empty list makes sense, because that is filtering. i.e. there are no items that match your criteria
Yes I think you have to ask why you got to there in the first place and how you will handle this kind of situation.
Kosmik
Honorary Master
Odd, can't say I've ever experienced this with entity. It generally tracks quite well and the persistence is never done to a object that has not changed regardless of reference or calls.
DA-LION-619
Honorary Master
You’re adding the Rental to the Booking, EF will consider that Entity(rental) in an Added state not modified because it isn’t tracking.
From an instance of a context, any reference to an entity is the same entity.
The entity would be tracked in an unchanged state.
Depending on your modelling, EF could generate an INSERT or MERGE(upsert) statement.
Solarion
Honorary Master
- Joined
- Nov 14, 2012
- Messages
- 28,059
- Reaction score
- 17,821
HI guys. Could you please give a little feedback on a design route I'm experimenting with.
I ditched repositories and unit of work and went with injecting the dbcontext directly into the service/application layer.
It works great. But I'm not sure if it is a sound way to do things and just needing your feedback please.
The reason this is niggling at me a little is because in the service layer I am still having to call Update and then SaveChanges according to EF requirements. So it feels like there is some coupling/dependency there even if just barely tangible.
Context (Persistence)
Middleware
Service/Application
I ditched repositories and unit of work and went with injecting the dbcontext directly into the service/application layer.
It works great. But I'm not sure if it is a sound way to do things and just needing your feedback please.
The reason this is niggling at me a little is because in the service layer I am still having to call Update and then SaveChanges according to EF requirements. So it feels like there is some coupling/dependency there even if just barely tangible.
Context (Persistence)
C#:
public class ApplicationDbContext : DbContext, IApplicationDbContext
{
private readonly ICurrentUserService _currentUserService;
private readonly IDateTimeService _dateTimeService;
public ApplicationDbContext(DbContextOptions<ApplicationDbContext> options, IDateTimeService dateTimeService, ICurrentUserService currentUserService) : base(options)
{
_dateTimeService = dateTimeService;
_currentUserService = currentUserService;
}
public DbSet<Job> Jobs { get; set; }
public override async Task<int> SaveChangesAsync(CancellationToken cancellationToken = new CancellationToken())
{
foreach (var entry in ChangeTracker.Entries<AuditableEntity>())
{
switch (entry.State)
{
case EntityState.Added:
entry.Entity.CreatedBy = _currentUserService.UserId;
entry.Entity.Created = _dateTimeService.Now;
break;
case EntityState.Modified:
entry.Entity.LastModifiedBy = _currentUserService.UserId;
entry.Entity.LastModified = _dateTimeService.Now;
break;
}
}
var result = await base.SaveChangesAsync(cancellationToken);
return result;
}
protected override void OnModelCreating(ModelBuilder builder)
{
base.OnModelCreating(builder);
foreach (var foreignKey in builder.Model.GetEntityTypes().SelectMany(e => e.GetForeignKeys()))
{
foreignKey.DeleteBehavior = DeleteBehavior.Restrict;
}
}
}Middleware
C#:
services.AddDbContext<ApplicationDbContext>(options =>
options.UseSqlServer(
configuration.GetConnectionString("DefaultConnection"),
b => b.MigrationsAssembly(typeof(ApplicationDbContext).Assembly.FullName)));
services.AddScoped<IApplicationDbContext>(provider => provider.GetRequiredService<ApplicationDbContext>());Service/Application
C#:
public class JobService : IJobService
{
private readonly IApplicationDbContext _context;
public JobService(IApplicationDbContext context)
{
_context = context;
}
public async Task<List<Job>> GetAsync()
{
return await _context.Jobs
.Include(x => x.JobType)
.ToListAsync();
}
public async Task<Job> GetByIdAsync(int id)
{
return await _context.Jobs
.Where(x => x.Id == id)
.Include(x => x.JobType)
.FirstOrDefaultAsync();
}
public async Task<Job> CreateAsync(CreateJobRequest request)
{
var entity = new Job()
{
JobNo = request.JobNo,
JobTypeId = request.JobTypeId,
StartDate = request.StartDate,
EndDate = request.EndDate,
};
_context.Jobs.Add(entity);
await _context.SaveChangesAsync();
return entity;
}
public async Task<Job> UpdateAsync(UpdateJobRequest request)
{
var entity = await _context.Jobs.FindAsync(request.Id);
entity.Id = request.Id;
entity.JobNo = request.JobNo;
entity.StartDate = request.StartDate;
entity.EndDate = request.EndDate;
entity.JobTypeId = request.JobTypeId;
_context.Jobs.Update(entity);
await _context.SaveChangesAsync();
return entity;
}
public async Task<Job> DeleteAsync(int id)
{
var entity = await _context.Jobs.FindAsync(id);
_context.Jobs.Remove(entity);
await _context.SaveChangesAsync();
return entity;
}
}
Last edited:
Kosmik
Honorary Master
The idea behind repositories etc is the separation of dB code to your actual business logic in the service classes. Also you are likely handling a lot more lines of redundant code ie context add etc. Personally haven't used the unit of work pattern but my preference is to always separate the dB stuff out so repos work well.
Also how are you handing unit testing with the amalgamation?
Personally i feel that is a leaky abstraction, as your gut feel tells you.
I have had a similar situation and solved it as follows.
I created a TransactionContext class that is a container for whatever i want to put in it. It implements IDisposable.
So in the services layer you can use a factory to instantiate the TransactionContext with the using {} pattern.
The factory DIs your EF contexts Into the TransactionContext. Then the TransactionContext.Dispose calls the EF contexts Update etc methods.
on mobile so the formatting is going to be k@k.
using (TransactionContext trx = factory.CreateContext())
{
DoStuff(trx);
}
This defers the EF closures to the TransactionContext while keeping service layer free of EF junk
class TransactionContext
{
public TransactionContext(efContexts) {}
public void Dispose()
{
efContexts.SaveWhatever();
}
}
DA-LION-619
Honorary Master
If you’re only using SQL Server (let’s be honest), which isn’t cheap then use it.
I’d let the SQL Server handle auditing if needed as it can clean it up and also provides flexibility in picking languages, frameworks etc.
DA-LION-619
Honorary Master
But why? EF Core has a pooling option and EF does transactions implicitly.
How do you know the transaction was committed if you’re calling SaveChanges() in the Dispose method?