Skip to content

refactor: streamline row and column data handling by implementing cac… #1841

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

Merged
merged 1 commit into from
Jul 8, 2025

Conversation

AIlkiv
Copy link
Contributor

@AIlkiv AIlkiv commented Jun 13, 2025

This PR introduces caching at the ColumnMapper level, which allowed the following improvements:

  • Removed the allColumns and columns properties from Row2Mapper, which were previously passed implicitly between methods.
  • Simplified the signatures of several methods.
  • Made the overall mapping logic more transparent, understandable, and reliable.

These changes reduce hidden dependencies and improve code readability.

@AIlkiv AIlkiv requested review from enjeck and blizzz as code owners June 13, 2025 20:00
@AIlkiv AIlkiv force-pushed the refactor/add-caching-columns branch from cd1da46 to 14b3e13 Compare June 13, 2025 20:02
@blizzz blizzz added the technical debt Technical issue label Jun 19, 2025
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

@AIlkiv thanks also for this PR. Not sure it was yet for review? Did not test but left some remarks, generally looks nice! There is a also a conflict pending.

public function findAll(array $tableColumns, array $columns, int $tableId, ?int $limit = null, ?int $offset = null, ?array $filter = null, ?array $sort = null, ?string $userId = null): array {
$this->setColumns($columns, $tableColumns);
$columnIdsArray = array_map(fn (Column $column) => $column->getId(), $columns);
public function findAll(array $showColumnIds, int $tableId, ?int $limit = null, ?int $offset = null, ?array $filter = null, ?array $sort = null, ?string $userId = null): array {
Copy link
Member

Choose a reason for hiding this comment

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

please update or remove the phpdoc block above

@@ -19,6 +19,7 @@
class ColumnMapper extends QBMapper {
protected string $table = 'tables_columns';
private LoggerInterface $logger;
private array $cacheColumn = [];
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider OCP\Cache\CappedMemoryCache? We use it in TableMapper.

Comment on lines 94 to 95
public function findMultiple(array $ids): array {
$qb = $this->db->getQueryBuilder();
$qb->select('*')
->from($this->table)
->where($qb->expr()->in('id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY)));
return $this->findEntities($qb);
return $this->findAll($ids);
Copy link
Member

Choose a reason for hiding this comment

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

might just remove the whole method and/or rename findAll when it expects a limitation anyway.

@AIlkiv AIlkiv force-pushed the refactor/add-caching-columns branch 4 times, most recently from 9469c35 to 911348c Compare June 26, 2025 19:43
@AIlkiv
Copy link
Contributor Author

AIlkiv commented Jun 26, 2025

@blizzz Thank you for the review. I’ve made the necessary code changes. Please take another look when you have a moment.

@AIlkiv AIlkiv requested a review from blizzz June 27, 2025 08:35
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!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

…hing and simplifying method signatures

Signed-off-by: ailkiv <a.ilkiv.ye@gmail.com>
@AIlkiv AIlkiv force-pushed the refactor/add-caching-columns branch from 911348c to e761f16 Compare July 8, 2025 09:14
@blizzz blizzz merged commit 720b69e into nextcloud:main Jul 8, 2025
68 of 77 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants