Skip to content

PATCH request with Nullable ID does not work. #467

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

Closed
czemanek-arm opened this issue Jan 28, 2019 · 8 comments
Closed

PATCH request with Nullable ID does not work. #467

czemanek-arm opened this issue Jan 28, 2019 · 8 comments

Comments

@czemanek-arm
Copy link

Description

Whenever a model inherits from Identifiable<T> and T is nullable (i.e. long?) PATCH requests are not able to be found.

This is related to my comment on #363 in that, after implimenting my work around there, I discovered that nullable ID's are unable to pick up PATCH requests in EF. POST and GET still work fine.

...

Environment

  • JsonApiDotNetCore Version: 3.0.1-alpha3
  • Other Relevant Package Versions:
@jaredcnance
Copy link
Contributor

Can you share your controller?

@czemanek-arm
Copy link
Author

czemanek-arm commented Feb 11, 2019

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Logging;

using JsonApiDotNetCore.Controllers;
using JsonApiDotNetCore.Models;
using JsonApiDotNetCore.Services;
using JsonApiDotNetCore.Data;

namespace Controllers
{
	[Authorize]
	[NoHttpDelete]
	public class BooksController : JsonApiController<Book, long?>
	{
		public BooksController(
			IJsonApiContext jsonApiContext,
			IResourceService<Book, long?> resourceService,
			ILoggerFactory loggerFactory
		: base(jsonApiContext, resourceService, loggerFactory) { }

		[HttpPost]
		public override async Task<IActionResult> PostAsync([FromBody] Book book)
		{
			return await base.PostAsync(book);
		}

		[HttpPatch("{id}")]
		public override async Task<IActionResult> PatchAsync(long? id, [FromBody] Book book)
		{
			return await base.PatchAsync(id, book);
		}

		[HttpPatch("{id}/relationships/{relationshipName}")]
		public override async Task<IActionResult> PatchRelationshipsAsync(long? id, string relationshipName, [FromBody] List<ResourceObject> relationships)
			=> await base.PatchRelationshipsAsync(id, relationshipName, relationships);
	}
}

Thanks

@jaredcnance
Copy link
Contributor

jaredcnance commented Feb 11, 2019

Thanks. Do you know if the controller is being invoked at all on PATCH? i.e. are you getting 404 for route not found or because the resource cannot be found in the data store?

@czemanek-arm
Copy link
Author

I am getting a 404, no breakpoints in the PATCH function were reached.

@jaredcnance
Copy link
Contributor

jaredcnance commented Feb 11, 2019

This sounds like an issue with the routing configuration and outside the influence of JADNC. You might try changing your route template to "{id?}". But, I'm curious what your reasoning is for choosing a nullable identifier? Disregard, I just read you comment here

@czemanek-arm
Copy link
Author

I'll look into that, but here's how I got there:

  • ID column is a big int that is an identity
  • PATCH doesn't work as some tables have rows of ID = 0 which gets turned into the empty string by JADNC
  • Modify Identifiable so GetStringId doesn't erase 0
  • Now whenever POST occurs, there's no empty value, so it tries to post a 0
  • Fails because table is identity (also 0 already exists)
  • Turn ID to nullable so its value can be null and thus empty
  • Causes PATCH to fail for some reason

Leads me to the current fix:

  • Inherit from Identifiable
  • Add bool field "IsPost"
  • Override GetStringId() => { return IsPost ? "" : id.toString() }
  • Set IsPost = true in POST request

Sorry, just saw your edit after I wrote it all out

@maurei
Copy link
Member

maurei commented Apr 29, 2019

@czemanek-arm If this is still relevant, I will look into this if you can provide me with reproducible case (a branch with tests that expose your issue)

@bart-degreed
Copy link
Contributor

Closing this, as it seems the discussion has stalled. Feel free to reopen if this still needs attention in v4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants