Wrong json returned from the api

[Leptopoda]

Description
like #1464 but this time with the recipe_stub it can be seen in the categories and recipe api endpoint.
adding a simple $json['recipe_id'] = strval($json['recipe_id']); to
https://github.com/Leptopoda/cookbook/blob/1fd2668f0ca1311b0585fda5effeeabea1f42d6a/lib/Helper/Filter/JSON/RecipeIdTypeFilter.php#L15
Did not fix it and my php was not good enough to find out why :)

Reproduction
see #1464

Expected behavior
recipe_id is a String

Actual behavior
recipe_id is an integer

Screenshots

Error: Deserializing '[{recipe_id: 248, name: Chef John's Gazpacho, keywords: null, dateCreated: 20...' to 'BuiltList<RecipeStub>'
failed due to: Deserializing '[recipe_id, 248, name, Chef John's Gazpacho, dateCreated, 2021-01-30 14:58:28...' to 'RecipeStub'
failed due to: Deserializing '248' to 'String' failed due to: type 'int' is not a subtype of type 'String' in type cast

Browser
n.a.

Versions
Nextcloud server version: 26.0.0
Cookbook version: 0.10.2
Database system: Sqlite and MariaDB

[arroyoj]

@Leptopoda, it looks like your fix for returning recipe IDs as strings doesn't work for the recipe stubs because the API endpoints that return recipe stubs do not pass the results through the RecipeJSONOutputFilter.

I think this is where the Recipe API returns the full recipe json, which it seems to read from the recipe.json file and then pass through the output filter (on line 94), where the RecipeIdTypeFilter can convert the recipe ID to a string:

cookbook/lib/Controller/Implementation/RecipeImplementation.php

Lines 82 to 97 in 3e3a9aa

	public function show($id) {
	$this->dbCacheService->triggerCheck();
	$json = $this->service->getRecipeById($id);
	if (null === $json) {
	return new JSONResponse($id, Http::STATUS_NOT_FOUND);
	}
	$json['printImage'] = $this->service->getPrintImage();
	$json['imageUrl'] = $this->urlGenerator->linkToRoute('cookbook.recipe.image', ['id' => $json['id'], 'size' => 'full']);
	$json = $this->outputFilter->filter($json);
	return new JSONResponse($json, Http::STATUS_OK);
	}

However, the recipe stub APIs query the database directly and return the results without going through the output filter. Here is one example, from the API for getting all of the recipes for a given category:

cookbook/lib/Controller/Implementation/RecipeImplementation.php

Lines 282 to 310 in 3e3a9aa

	public function getAllInCategory($category) {
	$this->dbCacheService->triggerCheck();
	$category = urldecode($category);
	try {
	$recipes = $this->service->getRecipesByCategory($category);
	foreach ($recipes as $i => $recipe) {
	$recipes[$i]['imageUrl'] = $this->urlGenerator->linkToRoute(
	'cookbook.recipe.image',
	[
	'id' => $recipe['recipe_id'],
	'size' => 'thumb',
	't' => $this->service->getRecipeMTime($recipe['recipe_id'])
	]
	);
	$recipes[$i]['imagePlaceholderUrl'] = $this->urlGenerator->linkToRoute(
	'cookbook.recipe.image',
	[
	'id' => $recipe['recipe_id'],
	'size' => 'thumb16'
	]
	);
	}
	return new JSONResponse($recipes, Http::STATUS_OK, ['Content-Type' => 'application/json']);
	} catch (\Exception $e) {
	return new JSONResponse($e->getMessage(), 500);
	}
	}

The corresponding DB query is here:

cookbook/lib/Db/RecipeDb.php

Line 252 in 3e3a9aa

public function getRecipesByCategory(string $category, string $user_id) {

My guess is the recipe ID from that query (and others) is being returned as an int (at least, the recipe ID is an int in my DB), and the API on your instance is never changing it to a string (although, on my instance of NC 25.0.6 with Cookbook 0.10.2, it is getting converted to a string, but I'm not sure why). I don't have a way to test this, and I'm not seeing the recipe IDs being returned as ints on my instance, so I'm not sure if I can help fix this directly, but hopefully that can help you fix it?

Maybe adding something like this to the foreach loop in getAllInCategory() would work?

$recipes[$i]['recipe_id'] = strval($recipe['recipe_id']);

If that works, it would probably need to be added to all the places where a recipe stub is returned, so maybe there is a better way to do it in one place.

[Leptopoda]

Oh thanks for the hint.
I didn't even dig deeper than trying to adapt the fix for #1464 to this one but it totally makes sense that the filter isn't even fired.
I'd need to have a look at the schema but I think all recipe_stub properties could be returned from the DB thus the filter wouldn't ever be needed.

I wonder if a DB migration would be better?
Sadly the maintainer said on matrix that he's quite occupied lately and I think we'd need to better ask/plan such a migration (especially as a big DB/backend restructure is already underway)

nevertheless thanks a lot for finding my mistakes :)

[arroyoj]

Digging into this a bit more, it looks like the underlying issue stems from a change between php-8.0 and php-8.1 that altered how the PDO-mysql driver returns numeric values from the database. Up to and including php-8.0, by default, integers were returned as strings from the database, so the recipe_id was returned as a string. Starting with php-8.1, the default for PDO-mysql changed to return database integers as native PHP integers, so for users on php-8.1 or later, the REST API will return recipe_id as an integer (passed through from the database call). Some additional details are here: Leptopoda/nextcloud_cookbook_dart_api#2 (comment).

I think the question now is whether this should be fixed at the database layer (ie, normalize the data coming out of RecipeDb so that recipe_id is always returned as a string) or at the controller level (so regardless of the underlying database model, the RecipeImplementation controller returns recipe_id as a string).

The unit tests covering the database layer rely on assertEquals() to check that the DB output is as expected, so those tests pass regardless of what php version is used. Basing the tests on assertSame() to check for identity, including the expected type, causes the tests to fail depending on which version of php is used. In some ways, that seems like the root issue and the database layer is the place to fix it, but the API spec only defines what should be returned from the API calls, not what the underlying database model should be.

On the other hand, the unit tests covering the RecipeImplementation controller rely on a mock of RecipeService, such that it never hits the database and returns whatever recipe_id type (string or int) is supplied in the test. In thinking of how to fix the API issue, I wanted to try first to update the tests of RecipeImplementation so they would initially fail on either new or old php versions and reveal the bug, but it doesn't seem like the controller is the right place for such a test. Maybe we need a separate test to make sure that regardless of the type of recipe_id that is provided by RecipeService, the controller always returns a string? This would essentially ignore the root DB issue, but ensure the API is consistent with the spec.

@christianlupus, it would be great to get your thoughts on where the fix for this issue belongs: the DB layer or the controller layer.