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

feat: decrease query count by cacheing some RDF property fields #1085

Conversation

mccar
Copy link
Contributor

@mccar mccar commented Nov 3, 2023

Ticket - https://oat-sa.atlassian.net/browse/REL-1279

Goal

Decrease query count by cacheing some RDF property fields

Changelog

  • feat: added cache for fields isLgDependent, isMultiple, isRelationship and control changes on set and delete data in database

How to test

  1. Debug \core_kernel_classes_Property on open items
  2. Run tao/scripts/taoInit.php to check warmup
  3. To check how to works method OntologyUpdater::modelSync you can add it to tao/scripts/taoInit.php because running it in an update depends on the version and does not run every time
  4. Unit tests for check cache for property fields \oat\generis\test\unit\core\kernel\classes\PropertyTest

Copy link
Contributor

@augustas augustas left a comment

Choose a reason for hiding this comment

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

I believe Redis is not a requirement to install TAO, generis, therefore this new property cache should fallback to what the SimpleCache is configured by default

helpers/class.PropertyCache.php Outdated Show resolved Hide resolved
core/kernel/classes/class.Property.php Outdated Show resolved Hide resolved
Copy link
Contributor

@augustas augustas left a comment

Choose a reason for hiding this comment

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

I approve the PR, but I would like you to create a service with CONST SERVICE_ID = 'generis/PropertyCache'; That is what would be more expected in TAO

Copy link
Contributor

@augustas augustas left a comment

Choose a reason for hiding this comment

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

When creating a new property via user interface, default value false is saved for Is language dependent?, and modifying the value to true does not update the property cache. At least in my manual test.
Also the PR needs a migration, to add config/generis/PropertyCache.conf.php, when an already installed instance is updated.

Copy link
Contributor

@yaraslau-kavaliou yaraslau-kavaliou left a comment

Choose a reason for hiding this comment

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

Overall solution looks good from me.
But during update of existing instance I see this issue:

tao.ERROR: Service "generis/PropertyCache" not found {"exception":"oat\\oatbox\\service\\ServiceNotFoundException","file":"/var/www/html/generis/common/oatbox/service/ServiceManager.php","line":139,"trace":[{"file":"/var/www/html/generis/common/oatbox/service/ServiceManager.php","line":115,"function":"tryAutowiring","class":"oat\\oatbox\\service\\ServiceManager","type":"->","args":["generis/PropertyCache","generis/PropertyCache"]},{"file":"/var/www/html/generis/common/oatbox/service/ServiceManager.php","line":85,"function":"load","class":"oat\\oatbox\\service\\ServiceManager","type":"->","args":["generis/PropertyCache","generis/PropertyCache"]},{"file":"/var/www/html/generis/core/kernel/persistence/smoothsql/class.SmoothModel.php","line":110,"function":"get","class":"oat\\oatbox\\service\\ServiceManager","type":"->","args":["generis/PropertyCache"]},{"file":"/var/www/html/generis/core/kernel/classes/class.Property.php","line":309,"function":"getCache","class":"core_kernel_persistence_smoothsql_SmoothModel","type":"->","args":[]},{"file":"/var/www/html/generis/core/kernel/classes/class.Property.php","line":125,"function":"isLgDependent","class":"core_kernel_classes_Property","type":"->","args":[]},{"file":"/var/www/html/tao/helpers/form/class.GenerisFormFactory.php","line":86,"function":"feed","class":"core_kernel_classes_Property","type":"->","args":[]},{"file":"/var/www/html/tao/actions/form/class.EditClassLabel.php","line":104,"function":"elementMap","class":"tao_helpers_form_GenerisFormFactory","type":"::","args":["[object] (core_kernel_classes_Property: http://www.w3.org/2000/01/rdf-schema#label\nLabel)"]},{"file":"/var/www/html/tao/helpers/form/class.FormContainer.php","line":119,"function":"initElements","class":"tao_actions_form_EditClassLabel","type":"->","args":[]},{"file":"/var/www/html/tao/actions/form/class.EditClassLabel.php","line":57,"function":"__construct","class":"tao_helpers_form_FormContainer","type":"->","args":[{"classUri":"https_2_premium_0_docker_0_localhost_1_ontologies_1_tao_0_rdf_3_i6569af282a416450b86aa386d2809dd","id":"https://premium.docker.localhost/ontologies/tao.rdf#i6569af282a416450b86aa386d2809dd"},{"csrf_protection":true,"is_disabled":false}]},{"file":"/var/www/html/tao/actions/class.RdfController.php","line":450,"function":"__construct","class":"tao_actions_form_EditClassLabel","type":"->","args":["[object] (core_kernel_classes_Class: https://premium.docker.localhost/ontologies/tao.rdf#i6569af282a416450b86aa386d2809dd\n)",{"classUri":"https_2_premium_0_docker_0_localhost_1_ontologies_1_tao_0_rdf_3_i6569af282a416450b86aa386d2809dd","id":"https://premium.docker.localhost/ontologies/tao.rdf#i6569af282a416450b86aa386d2809dd"},"33b1f11e131baf049fb01b12974e2bff0c3f3276934e3a570f8679eef212f1c2",{"csrf_protection":true,"is_disabled":false}]},{"function":"editClassLabel","class":"tao_actions_RdfController","type":"->","args":[]},{"file":"/var/www/html/tao/models/classes/routing/ActionEnforcer.php","line":257,"function":"call_user_func_array","args":[["[object] (taoItems_actions_Items: {})","editClassLabel"],[]]},{"file":"/var/www/html/tao/models/classes/routing/ActionEnforcer.php","line":207,"function":"resolve","class":"oat\\tao\\model\\routing\\ActionEnforcer","type":"->","args":["[object] (GuzzleHttp\\Psr7\\ServerRequest: {})"]},{"file":"/var/www/html/tao/models/classes/routing/ActionEnforcer.php","line":183,"function":"execute","class":"oat\\tao\\model\\routing\\ActionEnforcer","type":"->","args":[]},{"file":"/var/www/html/tao/models/classes/routing/TaoFrontController.php","line":106,"function":"__invoke","class":"oat\\tao\\model\\routing\\ActionEnforcer","type":"->","args":["[object] (GuzzleHttp\\Psr7\\ServerRequest: {})","[object] (GuzzleHttp\\Psr7\\Response: {})"]},{"file":"/var/www/html/tao/models/classes/mvc/Bootstrap.php","line":342,"function":"__invoke","class":"oat\\tao\\model\\routing\\TaoFrontController","type":"->","args":["[object] (GuzzleHttp\\Psr7\\ServerRequest: {})","[object] (GuzzleHttp\\Psr7\\Response: {})"]},{"file":"/var/www/html/tao/models/classes/mvc/Bootstrap.php","line":189,"function":"mvc","class":"oat\\tao\\model\\mvc\\Bootstrap","type":"->","args":[]},{"file":"/var/www/html/tao/models/classes/mvc/Bootstrap.php","line":249,"function":"dispatchHttp","class":"oat\\tao\\model\\mvc\\Bootstrap","type":"->","args":[]},{"file":"/var/www/html/index.php","line":29,"function":"dispatch","class":"oat\\tao\\model\\mvc\\Bootstrap","type":"->","args":[]}]} []

Installation from scratch works without issues.
And the issue which Augustas mentioned also true for me:

When creating a new property via user interface, default value false is saved for Is language dependent?, and modifying the value to true does not update the property cache

migrations/Version202312011658572348_generis.php Outdated Show resolved Hide resolved
core/kernel/persistence/Cache.php Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Dec 6, 2023

Version

Target Version 15.33.0
Last version 15.32.1

There are 0 BREAKING CHANGE, 13 features, 0 fix

@mccar mccar requested a review from augustas December 6, 2023 13:07
Copy link
Contributor

@augustas augustas left a comment

Choose a reason for hiding this comment

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

  • New code is covered by tests (if applicable)
  • Tests are running successfully (old and new ones) on my local machine (if applicable)
  • New code is respecting code style rules
  • New code is respecting best practices
  • New code is not subject to concurrency issues (if applicable)
  • Feature is working correctly on my local machine (if applicable)
  • Acceptance criteria are respected
  • Pull request title and description are meaningful
  • Changelog is updated according to changes (if applicable)
  • Documentation is updated according to changes (if applicable)

@mccar mccar merged commit c6f5e21 into develop Dec 7, 2023
5 of 6 checks passed
@mccar mccar deleted the feat/REL-1279/decrease-query-count-by-caching-some-rdf-property-fields branch December 7, 2023 08:46
/**
* Clear property cached data
*/
public static function clearCachedValuesByTriples(Iterator $triples): void
Copy link
Contributor

Choose a reason for hiding this comment

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

missing namespace

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to add use Iterator to line 4 (between namespace and class)

namespace oat\generis\Helper;

use Iterator;

class PropertyCache

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

Successfully merging this pull request may close these issues.

5 participants