Skip to content
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

fix(api): ContextMapper now coerces the row ID to an int for proper comparison #954

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

elzody
Copy link
Contributor

@elzody elzody commented Mar 26, 2024

Overview

I've been having an issue where, after I add a Context and try to re-fetch all Contexts from /ocs/v2.php/apps/tables/api/2/contexts/, the response returned by the API is a Context[] as expected, but the values are all null.

Example from before fix
{
    "data": [
        {
            "id": null,
            "name": null,
            "iconName": null,
	    "description": null,
	    "owner": null,
	    "ownerType": null,
	    "sharing": [],
	    "nodes": [],
	    "pages": []
        }
    ]
}

This issue was preventing me from properly reviewing pull requests, so I decided to look into it. The problem was found in ContextMapper.php, where the row IDs were of type string, and the context ID variable held ints. They were being compared using === operator which, in addition to value, checks for type equivalence. string and int are not of the same type, so the comparison was failing, resulting in null values.

Upon further investigation and with help from @juliushaertl, it is thought that it is due to sqlite storing the id as a string by default. The solution here would be to cast the row ID to an int so that the strict comparison succeeds and to maintain compatibility with sqlite.

@elzody elzody added bug Something isn't working 3. to review Waiting for reviews labels Mar 26, 2024
@elzody elzody requested review from blizzz, juliusknorr and enjeck and removed request for blizzz March 26, 2024 19:07
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should rather check why the row id is not an int here, I would expect that to be the case when read from the database, where it should be mapped automatically. I can help looking into that tomorrow

@elzody elzody requested a review from blizzz as a code owner March 27, 2024 14:55
Base automatically changed from feat/contexts to main March 27, 2024 16:35
@elzody elzody force-pushed the fix/contexts/null-context-props branch from 77d25ab to 99ca0db Compare March 27, 2024 16:42
…ity with sqlite database

Signed-off-by: Elizabeth Danzberger <lizzy7128@tutanota.de>
@elzody elzody force-pushed the fix/contexts/null-context-props branch from 99ca0db to 440b2cb Compare March 27, 2024 16:48
@elzody elzody requested a review from juliusknorr March 27, 2024 16:59
@juliusknorr juliusknorr merged commit 4826a10 into main Mar 27, 2024
57 checks passed
@juliusknorr juliusknorr deleted the fix/contexts/null-context-props branch March 27, 2024 17:33
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working feedback-requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants