When an attribute is indexed with matching rules from collation plugin, the indexer is not retrieved and indexing fails
This bug exists in 1.3.6 and 1.3.7
The failure was initially detected in I18n suite. It is reproducible with the attached test case
Indexing fails and warnings are logged in error logs
unknown or invalid matching rule
Indexing should succeeds and
<img alt="0001-Ticket-49509-Indexing-of-internationalized-matching-.patch" src="/389-ds-base/issue/raw/files/3554265c38893d81562954dead419fb25376804d7d8e8d872cb0b238b16209cf-0001-Ticket-49509-Indexing-of-internationalized-matching-.patch" />
Metadata Update from @tbordaz: - Custom field component adjusted to None - Custom field origin adjusted to None - Custom field reviewstatus adjusted to None - Custom field type adjusted to None - Custom field version adjusted to None
Metadata Update from @tbordaz: - Custom field reviewstatus adjusted to review (was: None)
<img alt="0002-Ticket-49509-Indexing-of-internationalized-matching-.patch" src="/389-ds-base/issue/raw/files/0bf9430be355a2e7b25b6f7de68fc2c813b56f4b0224ec3b3aee029f289e43bd-0002-Ticket-49509-Indexing-of-internationalized-matching-.patch" />
Copy/past of a review done by @lkrispen
First question
Second question
<img alt="0003-Ticket-49509-Indexing-of-internationalized-matching-.patch" src="/389-ds-base/issue/raw/files/58325ba2bfca76097c0c5491685b7d1eca94486cc1067d7f71ddb9f745940045-0003-Ticket-49509-Indexing-of-internationalized-matching-.patch" />
Copy/past of a last review part done by @lkrispen
Thank you for writing test case also. :)
A few points we need to improve:
Python 3 support: We need to use lib389 functions for it and avoid modify_s, add_s, etc. I've updated documentation for Indexes with examples and links to source code: https://fedorapeople.org/~spichugi/html/indexes.html https://fedorapeople.org/~spichugi/html/guidelines.html#python-3-support
Please, replace you code in function 'description_collation_indexed' with it.
Test file and function naming conventions: It's better to put new test cases directly to suites. It was decided that we need to get rid of tickets dir soon anyway. I think we can put the test case to something like 'suites/indexes/regression_test.py' (new directory)
Then you can rename the test function with something sensible like 'test_collation_utf8' instead of 'test_ticket49509'.
Docstring:
7850 + """Specify a test case purpose or name here 7851 + 7852 + :id: d2b02800-a903-4647-98e2-89e47d99df39 7853 + :setup: Fill in set up configuration here 7854 + :steps: 7855 + 1. Create a UTF-8 7856 + 2. Configure collation plugin indexes for 'description' attribute 7857 + 3. import UTF-8 file and check no error indexing 7858 + 4. dump 'description' index and check it contains collation keys 7859 + """
We need to write a small description in the first line. :setup: will be something like 'Standalone instance' Besides :steps:, please, fill in :expectedresults: too (it can be either one number '1. It should compile', or the number matching steps, if it makes more sense)
You can check any other test case in suites for an example: https://pagure.io/389-ds-base/blob/master/f/dirsrvtests/tests/suites/filter/filter_test.py#_20
Also, please, remove boilerplate code:
7869 + # If you need any test suite initialization, 7870 + # please, write additional fixture for that (including finalizer). 7871 + # Topology for suites are predefined in lib389/topologies.py. 7872 + 7873 + # If you need host, port or any other data about instance, 7874 + # Please, use the instance object attributes for that (for example, topo.ms["master1"].serverid) 7875 + 7876 + if DEBUGGING: 7877 + # Add debugging steps(if any)... 7878 + pass
@tbordaz There are a number of object oriented types for things like indexes on backends and others.
As well, I'd rather not see raw lidfs in the source: They could be python objects in the test case for our purposes, and even better, would be to add some of these data types to the dbgen.py in lib389 so we can test them in "many other" cases besides just this one,
If you need some examples, I can link them, but I concur with @spichugi's review :)
Thanks!
Metadata Update from @mreynolds: - Custom field rhbz adjusted to https://bugzilla.redhat.com/show_bug.cgi?id=1531153
Issue linked to Bugzilla: Bug 1531153
patch looks good to me now, maybe you split the fix and the testcase as tehre seem to be some requests regarding the testcase
I agree, like other pending ticket I was working on, patch will be split and testcase push will be postponed until testcase is python3.
Do you ack the C part of the patch ?
Note I will open a doc BZ as SLAPI_PLUGIN_MR_INDEX_SV_FN was missing in external doc.
Agreed, similar to the other case, you can merge C and test separately (but please don't forget about the tests :) )
Metadata Update from @lkrispen: - Custom field reviewstatus adjusted to ack (was: review)
To ssh://pagure.io/389-ds-base.git d3ba228..3a161b2 master -> master
Metadata Update from @tbordaz: - Issue close_status updated to: fixed - Issue set to the milestone: 1.3.7 backlog - Issue status updated to: Closed (was: Open)
Metadata Update from @tbordaz: - Issue set to the milestone: 1.3.6.0 (was: 1.3.7 backlog) - Issue status updated to: Open (was: Closed)
Missing part of the current testcase <img alt="Backends.UTF-8.ldif" src="/389-ds-base/issue/raw/files/720e876ddf4d37d98ec31e34d65c29195c2f5a61fc503d947207e1685a76bc2f-Backends.UTF-8.ldif" />
Metadata Update from @tbordaz: - Issue close_status updated to: fixed
To ssh://pagure.io/389-ds-base.git 689b388..26f37b8 389-ds-base-1.3.6 -> 389-ds-base-1.3.6
Metadata Update from @tbordaz: - Issue status updated to: Open (was: Closed)
Metadata Update from @tbordaz: - Issue close_status updated to: fixed - Issue status updated to: Closed (was: Open)
To ssh://pagure.io/389-ds-base.git a2df295..4316907 389-ds-base-1.3.7 -> 389-ds-base-1.3.7
This is causing a serious regression, all server side sorting crashes the server with this patch.
Metadata Update from @mreynolds: - Issue assigned to tbordaz - Issue status updated to: Open (was: Closed)
int matchrule_values_to_keys(Slapi_PBlock *pb, struct berval **input_values, struct berval ***output_values) { IFP mrINDEX = NULL; slapi_pblock_get(pb, SLAPI_PLUGIN_MR_INDEX_FN, &mrINDEX); slapi_pblock_set(pb, SLAPI_PLUGIN_MR_VALUES, input_values); if (mrINDEX) { mrINDEX(pb); slapi_pblock_get(pb, SLAPI_PLUGIN_MR_KEYS, output_values); return LDAP_SUCCESS; } else { return LDAP_OPERATIONS_ERROR; } }
The crash happens because mrINDEX is not set, it returns an operations error and leaves the values NULL, which later get dereferenced
Where does it get dereferenced (do you have a stack trace/test case)? It seems that if this (validly) sets an operation error, we should be able to handle that.
So either we need to NOT return an op-error here, or we need the caller to handle that mrINDEX could be NULL.
-- Sincerely,
William
@tbordaz - what is the state of this issue? Was this ever fixed? Can it be closed?
Metadata Update from @mreynolds: - Issue set to the milestone: 1.4.2 (was: 1.3.6.0)
@mreynolds, the fix for this ticket has been pushed upstream since 1.3.6.
A possible regression (https://pagure.io/389-ds-base/issue/49509#comment-525208) is missing some details. In fact, server side sorting is working for me so I can not reproduce it. In addition all the calls to matchrule_values_to_keys look safe as output_values is initialized to NULL so if the function fails to retrieve a MR then output_values remains NULL an caller seem to handle it correctly.
So IMHO the remaining part of the ticket is the testcase. I wrote it 2 years ago but it was python2 style and I need to rewrite it.
@mreynolds, the fix for this ticket has been pushed upstream since 1.3.6. A possible regression (https://pagure.io/389-ds-base/issue/49509#comment-525208) is missing some details. In fact, server side sorting is working for me so I can not reproduce it. In addition all the calls to matchrule_values_to_keys look safe as output_values is initialized to NULL so if the function fails to retrieve a MR then output_values remains NULL an caller seem to handle it correctly. So IMHO the remaining part of the ticket is the testcase. I wrote it 2 years ago but it was python2 style and I need to rewrite it.
Cool, well when you rewrite the CI test please feel free to close this ticket. Thanks!
Metadata Update from @vashirov: - Issue priority set to: normal - Issue set to the milestone: 1.4.3 (was: 1.4.2)
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/2568
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix - Issue status updated to: Closed (was: Open)
Login to comment on this ticket.