PR - Ticket 50980 - RFE extend usability for slapi_compute_add_search_rewriter and slapi_compute_add_evaluator

[389-ds-bot]

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50981

• Created at 2020-03-24 17:05:36 by tbordaz (@tbordaz)
• Merged at 2020-04-14 13:56:24

---

Bug Description: plugin api allows to register filter rewriter callback (slapi_compute_add_search_rewriter) and computed attribute callback (slapi_compute_add_evaluator) This requires to write a new plugin to register callbacks. This RFE is to simplify the use of those plugin api interfaces so that rewriters (filter or computed attribute) in shared library can be taken into account as soon as listed in config entries

Fix Description: It follows the design http://www.port389.org/docs/389ds/design/search_rewriters.html registers callback listed in children of 'cn=rewriters,cn=config' The rewriters.c files contains examples of filter rewriter and computed attribute

Resolves: #4033

Reviewed by: Mark Reynolds, Alexander Bokovoy, William Brown (thanks !!)

Platforms tested: F30

Flag Day: no

Doc impact: no

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2020-03-24 18:29:35

should be size_t, and declared inside of the for loops

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2020-03-24 18:31:20

You don't need the Sun Micro copyright, and you can use 2020 for Red Hat's :-)

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2020-03-24 18:34:37

This indentation is off ;-)

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2020-03-24 18:35:47

Indentation is funny here as well.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2020-03-24 18:36:28

Indentation...

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2020-03-24 18:37:28

Most of the logging calls have incorrect indentation on the wrapped line - I'll stop commenting on each one.

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-03-24 18:54:56

rebased onto 07f68cfd25f645ff804e40b57edff6257a924424

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-03-24 19:29:59

@mreynolds389 thanks for your review. I made the changes you reported.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-03-25 00:37:37

I think all this debugging boilerplate isn't needed now, topologies does it for you behind the scenes.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-03-25 00:38:12

Shouldn't this be older than 1.4.4? I think it's a less than check, not less than equal to.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-03-25 00:38:54

UserAccounts has a "create test user" helper. See src/lib389/lib389/idm/user.py

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-03-25 00:39:38

I don't think anyone runs these directly now, so we probably don't nee the name anymore?

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-03-25 00:40:03

int32_t

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-03-25 01:23:07

Maybe I'm not awake enough yet, where are the rewriters called from in the search path?

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-03-25 11:44:41

rebased onto bfc7eb05e1f3e1ab63da39ea8bcdd321798d56e6

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-03-25 11:45:17

@mreynolds389 @Firstyear thanks for your reviews. Patch updated

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-03-26 00:08:17

int32_t here

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-03-26 00:08:55

And here

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-03-26 00:09:12

And this comment thierry:

Maybe I'm not awake enough yet, where are the rewriters called from in the search path?

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-03-26 00:09:29

Thanks by the way, it looks interesting :)

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-03-26 12:12:34

rebased onto 39ed68fd9c01ac65888e608c0c8ec3bd56503ca6

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-03-26 13:15:02

rebased onto f89639626ae81328bb23d442a01690b51faa3901

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-03-26 13:34:22

The rewriters are quite hidden in search path. They are called with compute_rewrite_search_filter (opshared.c) and compute_attribute (result.c).

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-03-27 01:45:46

So to be sure, these are only loaded from plugins at startup? These won't be "dynamic" like other plugins (This is a good thing that they are loaded at startup only IMO, dynamic adds a lot of complexity.).

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-03-27 11:00:29

Theorically slapi_compute_add_search_rewriter and slapi_compute_add_evaluator) can be called at anytime during server lifetime. So a new rewriter could be register anytime.

Currently most rewriters are registered at startup (compute_init, entry_computed_attr_init, ldbm_back_start). An exception is view plugin (views_start) that may register several times the same filter rewriter. The view rewriter looks safe to be registered several times. The only drawback is that it will be called several times.

I agree it is better to register the new rewriters only are startup (less complexity). Another good reason to make the framework in core server than in a plugin ;)

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-03-30 01:09:15

I agree it is better to register the new rewriters only are startup (less complexity). Another good reason to make the framework in core server than in a plugin ;)

Agreed. It's much simpler to accept the server restart, than to try to make these dynamic :) and they'll perform better too as a result!

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-03-30 01:09:50

As a general aside, this PR and your objectCategory PR could be a single PR so we can see the pieces and interactions of both at once maybe? Is there a benefit to splitting them?

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-03-30 09:22:25

Yes I prefer to separate them as it was source of confusion in the previous PR (#3992) that delivered the new mechanism to register rewriters and a specific rewriter (objectCategory).

we can anticipate several specific rewriters (objectCategory, objectSID, uniquemember,..) that could be part of 389-ds or potentially others projects (freeipa). Rewriters are independent of the new registering mechanism.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-03-31 00:58:18

I think there are some improvements you could make between #4041 and this if they were together, as it gives you the full picture?

Anyway, thanks for your patience on this, I appreciate you taking the time on all my comments :)

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-04-01 18:51:36

rebased onto a534d46552ae505ba4e0dac2297442c695e4da21

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-04-01 18:52:24

@Firstyear, please have a look at the rebase. Thanks

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-04-02 05:13:41

Use topology_st.standalone.ds_paths.lib_dir instead here. I think you can have:

libslapd = os.path.join( topology_st.standalone.ds_paths.lib_dir, 'dirsrv/libslapd.so')

Which would be much more reliable as a path lookup.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-04-02 05:15:59

You get the [0] because you are globbing, but if you just use the example I gave above, you don't need the array indexing.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-04-02 05:16:23

int!!!! int32_t please, like every int should be sized! :)

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-04-02 05:18:22

Better idea than 0 or -1 because that's so oooooo hard to track down in the future, is to use an enum instead. have a look at: #2638#_30__26

you can even give them int values etc. This way it's much easier to match what int means what, you have to use the enum variants, and the compiler can check when you are returning the wrong type.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-04-02 05:19:47

This goto isn't needed?

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-04-06 17:52:22

rebased onto 86604e4ce4d05d7998f452f818dae325ae8dc67a

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-04-06 17:54:39

@Firstyear the patch is updated according to your comments. Thanks

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-04-08 10:57:19

@Firstyear, I would ideally like to merge it this week, do you have any other concern regarding this PR ?

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-04-09 01:14:26

int ........

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-04-09 01:14:41

int ....

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-04-09 01:14:59

int ...

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-04-09 01:16:15

sizeof (entry_string) - 1

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-04-09 01:16:35

= {0}; to zero the stack

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-04-09 01:16:47

int.....

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2020-04-09 01:17:59

Besides these little comments, I think it's good. I was more thoroguh in the type check today :)

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-04-09 14:32:56

This one was on purpose it is a callback for slapi_filter_apply that is 'typedef int (FILTER_APPLY_FN)(Slapi_Filter f, void *arg);' I wanted to stick on the definition even if I guess int32_t would be accepted as well

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-04-09 14:35:05

Idem for error_code and rc that stick to int slapi_filter_apply(struct slapi_filter f, FILTER_APPLY_FN fn, void arg, int *error_code)

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-04-09 14:38:15

similar it is a callback for each returned entry of a search (slapi_search_internal_callback)

typedef int (*plugin_search_entry_callback)(Slapi_Entry *e,  void *callback_data);

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-04-09 14:42:05

It is used to retrieve pblock[SLAPI_PLUGIN_INTOP_RESULT] and sets a 'int', this is why I declared it as an int. Should it preferably be int32_t ?