Looking for urgent help with API

Solarion

Solarion

Honorary Master
Joined
Nov 14, 2012
Messages
28,052
Reaction score
17,805
I have this small API that I've been working on for a few days. I've hit a snag with it and I have no idea how to resolve this error. It has something to do with the way I have defined my classes and with Entity Framework.

This is a simple Rental and Booking API. You add a rental for a property and then add bookings against that rental.

I am getting the following error when attempting to add a booking:

An item with the same key has already been added. Key: 1
Click to expand...

I can either share the Git repo here or PM it. I've been on this for most of the day and don't know enough about Entity Framework to resolve this. Any help would be amazing guys.
 
Make sure you dont have duplicate properties in the model/base or use a reserved name somewhere as a property.
Maybe comment out the latest bunch you added and add them back bit by bit to see if you find the culprit.
 
I'll try that thanks B-1

Just going to hit it on Github and link it here anyway. You do not need to run any database, this uses EntityFramework InMemoryDatabase. Please feel free to see what you can find because my experience with EF is somewhat limited.

The problem is really made apprent in the BookingService when trying to book. This is where the error is being thrown.

C#: 
Booking booking = new Booking()
{
    Nights = request.NumberOfNigths,
    Start = request.StartDate.Date,
    Rental = rental
};

await _unitOfWork.BookingRepository.AddAsync(booking);
await _unitOfWork.Complete();

I have added a decent Swagger UI. The way it works is you create a Rental which represents say a bedroom to rent out:

Code: 
{
  "units": 1,
  "preparationTimeInDays": 1
}

And then you add a booking for that room which is 1, 2 or 3 days.

Code: 
{
  "rentalId": 1,
  "start": "2021-12-08",
  "nights": 3
}

Don't worry too much about how it works, I can explain it in more detail but that is it in a nutshell, a simple holiday booking system.
 
Last edited:
I suspect the problem may be with either the Rental or Booking table.

I have added xUnit tests however this issue is causing 3/4 to fail.
 
Last edited:
The_Ogre said:
Looks like a database issue. You're trying to insert a duplicate key in some table
Click to expand...
Not having looked at the code, I would put my bets on the "rentals" table

"rentalId": 1
Click to expand...
@ OP - a cheap and nasty way to debug is to use different numbers. You are using 1 for "preparationTime", "units", and "rentalId"

If you make them 1, 2 and 3, your error message will point you to which one is wrong.
 
scudsucker said:
Not having looked at the code, I would put my bets on the "rentals" table


@ OP - a cheap and nasty way to debug is to use different numbers. You are using 1 for "preparationTime", "units", and "rentalId"

If you make them 1, 2 and 3, your error message will point you to which one is wrong.
Click to expand...

Yeah the Rentals table I figured so too, I'm doing some processing of elimination at the moment but not getting very far lol!

I will try that thanks.

Edit: Excuse the somewhat untidiness of the project layout, I've yet to clean it up and get some proper order in there.
 
Last edited:
scudsucker said:
Not having looked at the code, I would put my bets on the "rentals" table


@ OP - a cheap and nasty way to debug is to use different numbers. You are using 1 for "preparationTime", "units", and "rentalId"

If you make them 1, 2 and 3, your error message will point you to which one is wrong.
Click to expand...

Looks like I cracked it. The tests are passing. I had to do a workaround though. I'll explain after I've uploaded to git.

Essentially I had to remove the Rental property from the Bookings table and just have a RentalId in the bookings table.

I then created another class to bridge the gap between Rental and Bookings.

Code: 
public class BookingsAndRentals
{
    public int Id { get; set; }
    public Rental Rental { get; set; }
    public DateTime StartDate { get; set; }
    public int NumberOfNights { get; set; }
}

And the literally just iterate through bookings and populate it as such.

C#: 
var bookings = await _bookingService.GetAll();
bookings = bookings.Where(x => x.RentalId == request.RentalId);

List<BookingsAndRentals> BookingsAndRentals = new List<BookingsAndRentals>();

foreach (var booking in bookings)
{
    BookingsAndRentals.Add(new BookingsAndRentals()
    {
        Id = booking.Id,
        NumberOfNights = booking.Nights,
        StartDate = booking.Start,
        Rental = await _rentalService.GetByRentalId(request.RentalId),
    });
}

I have no idea why but it seems the Rental property in a Bookings table was causing it to give that duplicate entry error.
 
I have one last small question guys and then I'm done. If I modified this endpoint:

C#: 
[HttpPost]
public ResourceViewModel Post(BookingModel model)
{
.
.
.
}

And added threading:

C#: 
[HttpPost]
public async Task<ResourceViewModel> Post(BookingModel model)
{
.
.
.
}

Would that be breaking any of the following rules?
Please do not modify the API contract
- Do not rename the exposed model's properties
- Do not modify the API routes
Click to expand...
 
I'd argue, "no" - it is not modifying the contract, and it is certainly not modifying the routes.
 
Perfect, thanks scud. I'd hate to go and have to modify the whole project and revert it back.
 
I would not just change it to async. There are existing client calls to these apis and they do not do async calls. It s correct that it should be async but with existing non-async client calls in place I would not change it to async. Async changes the return type. I would set up some test code to learn about what happens and google about it 1st before making it async, I would suggest changing it to async as well as fixing the calling code to be async (await it).
 
Solarion said:
As long as changing them to Tasks is not modifying the API contract or the API Routes then it should be safe. The client was very specific about that.
Click to expand...
It does modify the contract. The return type is part of the contract. I would not risk making it all async for this task. Keep it non-async and then suggest another task to make it all async client calls included.
docs.microsoft.com

Because this call is not awaited, execution of the current method continues before the call is completed - Visual Basic

Learn more about: BC42358: Because this call is not awaited, execution of the current method continues before the call is completed
docs.microsoft.com
 
Benedict A55h0le said:
It does modify the contract. The return type is part of the contract. I would not risk making it all async for this task. Keep it non-async and then suggest another task to make it all async client calls included.
docs.microsoft.com

Because this call is not awaited, execution of the current method continues before the call is completed - Visual Basic

Learn more about: BC42358: Because this call is not awaited, execution of the current method continues before the call is completed
docs.microsoft.com
Click to expand...

I'll play it safe then and not use async. Thanks!
 
Thanks for the info guys much appreciate!
 
Right up the stack all the way to the DBContext I'm using and asynchronous calls. Should I abandon everything all the way or is there a way to preserve them and just leave the controllers as without asynchronous?

Been up all night, my grammar is starting to take a hit.
 
Solarion said:
Right up the stack all the way to the DBContext I'm using and asynchronous calls. Should I abandon everything all the way or is there a way to preserve them and just leave the controllers as without asynchronous?

Been up all night, my grammar is starting to take a hit.
Click to expand...
Async doesn’t not affect the API contract because it deals with responses.
When in doubt trust in @_kabal_
Benedict A55h0le said:
It does modify the contract. The return type is part of the contract. I would not risk making it all async for this task. Keep it non-async and then suggest another task to make it all async client calls included.
docs.microsoft.com

Because this call is not awaited, execution of the current method continues before the call is completed - Visual Basic

Learn more about: BC42358: Because this call is not awaited, execution of the current method continues before the call is completed
docs.microsoft.com
Click to expand...
Dumb. Return yourself to the bakery.
Spacerat said:
You also need to change the method name to *Async to keep with convention
Click to expand...
There is no convention, async will not magically make things faster, it can actually make perf slower.
It does allow you to handle more requests/s which is the actual use case here, no one has async and non-async endpoints.
 
Last edited:
You must log in or register to reply here.
Back
Top
Sign up to the MyBroadband newsletter
X