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

enh(API): add OCS API to create rows #1161

Merged
merged 3 commits into from
Jun 26, 2024
Merged

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Jun 21, 2024

solves #1105

also features
✨ completed and flexible PermissionMiddleware
✨ integration tests of new endpoint against both tables and views, owners and sharees, negative tests
✨ the advertised new OCS endpoint for creating rows
✨ helper model to avoid a loose array with set or not set keys flying around
✨ simple conversion helper to translate stringy node types to ints and other way around

Open question: should I create endpoints for all other row-related methods as well? Would make sense, this feels incomplete. Can do a follow up as well of course.

…perms

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz added the 3. to review Waiting for reviews label Jun 21, 2024
@blizzz
Copy link
Member Author

blizzz commented Jun 21, 2024

Just realized, there is a good number of added lines. But worry not:

  • ~37% are OpenAPI specs
  • ~37% are Integration tests
  • <30% is production code (and even there are lotta comments)

@blizzz blizzz force-pushed the enh/1105/create-row-ocs branch 3 times, most recently from 66cae89 to e69e065 Compare June 24, 2024 14:34
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
Copy link
Member Author

blizzz commented Jun 24, 2024

One set of failing tests were about gracefullness – fixed.

The other, because in 26 there is no NoAdminRequired attribute, only the annotation. Fixed the compatibility thing by adding it. We may consider nevertheless to drop 26 from 0.8.

@blizzz
Copy link
Member Author

blizzz commented Jun 26, 2024

btw,

Open question: should I create endpoints for all other row-related methods as well? Would make sense, this feels incomplete. Can do a follow up as well of course.

would only do as follow up, if only to not make this PR bigger

@blizzz blizzz added the enhancement New feature or request label Jun 26, 2024
@juliushaertl
Copy link
Member

Open question: should I create endpoints for all other row-related methods as well? Would make sense, this feels incomplete. Can do a follow up as well of course.

would only do as follow up, if only to not make this PR bigger

Yes, makes sense to line up for implementation I'd say, but let's keep this separate.

}
}
return true;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a use case for returning true in the assert-Methods if we throw anways in case of no permission?

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise I could not chain them in the match block: https://github.com/nextcloud/tables/pull/1161/files#diff-58a7c8ba568eef76405b8471729c4b735def0967160958d5ab0105ddb7afb34eR109-R112

could turn it into switch construction instead of course. I find the match more compact and readable.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, that makes sense then 👍

Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Cool, just a non-blocking question 👍

@juliushaertl juliushaertl merged commit 6cf91e4 into main Jun 26, 2024
58 checks passed
@juliushaertl juliushaertl deleted the enh/1105/create-row-ocs branch June 26, 2024 16:56
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 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants