Still reviewing, but we should have a design doc for this on the wiki.

@mreynolds, thanks for the review. Note it is still WIP. Would like to add a testcase and do some cleanup (for example config attribute attr/oid are likely useless).
I agree I have to write a design doc for this. This is also a reason why it is with this flag ;)

In AD the rewrite applied to any value -- anything in (objectCategory=foo) will get expanded to (objectCategory=cn=foo,cn=Schema,...). I don't see why you should get this limited by a set of values.

Thanks @abbra I misinterpret the document ! I will fix that along with the testcase

Why are you redeclaring this rather than loading it from proto-slap.h?

size_t for array indexing

= {0}; to zero the stack buffer.

I don't like that we have Func (symbol names) in the config - this plugin should be in a boolean state, enabled or disabled, and that functionality should just be encoded into the functions. We shouldn't be needing symbol names in the config here because that will make upgrade/downgrade/functionality changes harder.

Second, a benefit to just having it hardcoded, is no need for a config parser, config lock or anything else. Why add configuration until we actually need it? (and here we don't it's just "do what AD does". )

I think that you should have some lib389 tests to accompany this - first to make your own development lifecycle easier since you'll have easier testing capability, but also so we can "see" what the user/deployment experience will be like.

A question, will this plugin have any use case outside of freeipa?

@abbra, @mreynolds, @firstyear , thanks for you your initial review. I updated the patch accordingly and added a testcase. I remove the WIP flag as the patch is ready for review.

I will be writing a short design about that plugin but just giving some rational of it.
Thanks to slapi_compute_add_search_rewriter we can register rewriter callback. A limitation is that a developer need to write a new plugin to register it.

The main part of the patch is to create a "generic" plugin that takes callbacks from configuration (subentry::nsslapd-pluginRewriteFunc). So a developer "just" need to write rewriter callback, build it into a shared library, install the library (/lib/dirsrv/plugins) and create a subentry.

A second part is to use this generic plugin library to delivery simple rewriter callbacks (e.g. adfilter_rewrite_objectCategory)

The patch could be enhanced with a 'nsslapd-pluginComputeFunc' attribute for the subentries.
It would allow to register computed attribute callback like describe in https://pagure.io/389-ds-base/issue/137#comment-632080

I will be writing a short design about that plugin but just giving some rational of it.
Thanks to slapi_compute_add_search_rewriter we can register rewriter callback. A limitation is that a developer need to write a new plugin to register it.
The main part of the patch is to create a "generic" plugin that takes callbacks from configuration (subentry::nsslapd-pluginRewriteFunc). So a developer "just" need to write rewriter callback, build it into a shared library, install the library (/lib/dirsrv/plugins) and create a subentry.
A second part is to use this generic plugin library to delivery simple rewriter callbacks (e.g. adfilter_rewrite_objectCategory)

So this is a plugin that can call other plugins, but the server has to load them and ... okay sorry, but this is too complicated, and I'm going to now firmly say I won't accept this. There is no reason to have this kind of "dynamic" capability for "developers". Either they add the functionality into this plugin and we control it with a series of "on/off" behaviour booleans, or they make a seperate rewrite plugin, and we control the behaviour with the plugin is enabled/disabled. This whole dynamic symbol loading callback behaviour is a nightmare to debug, it's a pain to maintain, and it solves literally zero problems and instead adds complexity to our configuration and developer workload. I really think this aspect of this plugin needs reconsideration, and I won't accept this in it's current state unless there is a significantly stronger justification provided for why the design is measurably better than the behaviour boolean or multiple plugin solution I have provided.

We have a huge history of tech debt in our code because people over engineered solutions, and created code that was so complex and fragile that it's hard to change, and I'm not willing to see that continue, we need to be looking at simpler, focused, and maintainable solutions that just solve the problems at hand rather than being a "framework" to solve problems we don't have yet.

@firstyear thanks for sharing your concerns. I agree that we should care about server robustness/maintainability and complexity is not a friend. I think some of your concerns may be due to some unclear comments in the code and lack of design:

The patch delivers only one plugin with one nsSlapdPlugin config entry. Subentries are not nsSlapdPlugin. So this is not a plugin that call others plugins but a plugin that call shared library functions (rewriters/computed). We are responsible of making the plugin rock solid as well as rewriters we are delivering with it. We are not responsible of external rewriters.

389-ds offers a plugin-api. It has always been a challenge to diagnose a problem with an external plugin. I do not see why this new plugin extend that risk. If we have a crash with an external plugin or an external shared library it is similar.

I agree that filter rewriter and computed attributes are not a common use. But it can solve some problems and it is supported by plugin-api anyway. The generic plugin is just to make them easier to deploy.

You are right that 389-ds should not increase its complexity because of specific use. This is exactly the reason why I proposed that generic plugin. It is simple and is flexible enough to bring complexity outside of 389-ds. For example complex rewriter could be implemented/delivered by others projects (like freeipa) without the need to change 389-ds.

I will write down the design and if the generic plugins appears to be too fragile to be in 389-ds we still have the option to not configure it by default (like addn-plugin) or let other project deliver it.

@firstyear thanks for sharing your concerns. I agree that we should care about server robustness/maintainability and complexity is not a friend. I think some of your concerns may be due to some unclear comments in the code and lack of design:

The patch delivers only one plugin with one nsSlapdPlugin config entry. Subentries are not nsSlapdPlugin. So this is not a plugin that call others plugins but a plugin that call shared library functions (rewriters/computed). We are responsible of making the plugin rock solid as well as rewriters we are delivering with it. We are not responsible of external rewriters.

389-ds offers a plugin-api. It has always been a challenge to diagnose a problem with an external plugin. I do not see why this new plugin extend that risk. If we have a crash with an external plugin or an external shared library it is similar.

I agree that filter rewriter and computed attributes are not a common use. But it can solve some problems and it is supported by plugin-api anyway. The generic plugin is just to make them easier to deploy.

You are right that 389-ds should not increase its complexity because of specific use. This is exactly the reason why I proposed that generic plugin. It is simple and is flexible enough to bring complexity outside of 389-ds. For example complex rewriter could be implemented/delivered by others projects (like freeipa) without the need to change 389-ds.

I will write down the design and if the generic plugins appears to be too fragile to be in 389-ds we still have the option to not configure it by default (like addn-plugin) or let other project deliver it.

@firstyear thanks for sharing your concerns. I agree that we should care about server robustness/maintainability and complexity is not a friend. I think some of your concerns may be due to some unclear comments in the code and lack of design:

The patch delivers only one plugin with one nsSlapdPlugin config entry. Subentries are not nsSlapdPlugin. So this is not a plugin that call others plugins but a plugin that call shared library functions (rewriters/computed). We are responsible of making the plugin rock solid as well as rewriters we are delivering with it. We are not responsible of external rewriters.

389-ds offers a plugin-api. It has always been a challenge to diagnose a problem with an external plugin. I do not see why this new plugin extend that risk. If we have a crash with an external plugin or an external shared library it is similar.

I agree that filter rewriter and computed attributes are not a common use. But it can solve some problems and it is supported by plugin-api anyway. The generic plugin is just to make them easier to deploy.

You are right that 389-ds should not increase its complexity because of specific use. This is exactly the reason why I proposed that generic plugin. It is simple and is flexible enough to bring complexity outside of 389-ds. For example complex rewriter could be implemented/delivered by others projects (like freeipa) without the need to change 389-ds.

I will write down the design and if the generic plugins appears to be too fragile to be in 389-ds we still have the option to not configure it by default (like addn-plugin) or let other project deliver it.

@firstyear thanks for sharing your concerns. I agree that we should care about server robustness/maintainability and complexity is not a friend. I think some of your concerns may be due to some unclear comments in the code and lack of design:
The patch delivers only one plugin with one nsSlapdPlugin config entry. Subentries are not nsSlapdPlugin. So this is not a plugin that call others plugins but a plugin that call shared library functions (rewriters/computed). We are responsible of making the plugin rock solid as well as rewriters we are delivering with it. We are not responsible of external rewriters.

We are responsible for them, and history has shown we will be on the hook for it.

Remember as well, the GSS / support staff have to understand how this works. Can you explain it to them easily? Can they support it? Analyse it? Trace it?

389-ds offers a plugin-api. It has always been a challenge to diagnose a problem with an external plugin. I do not see why this new plugin extend that risk. If we have a crash with an external plugin or an external shared library it is similar.

This isn't the problem - the problem is your design and code here is doing dynamic symbol loading and use. I would prefer you create 30 plugins for rewriting stuff, than having a generic rewrite dynamic framework. We have a plugin framework. We don't need a "rewrite framework" inside our plugin framework, we need rewrite plugins in the framework and machinery we already have.

A generic framework is solving a problem that does not exist yet. The problem at hand is doing some AD compatible transforms. Focus on solving that, not on making "frameworks" to solve issues that don't exist yet. Complex solutions like this are not good solutions. That kind of thinking leads to unmaintainable support disasters.

I agree that filter rewriter and computed attributes are not a common use. But it can solve some problems and it is supported by plugin-api anyway. The generic plugin is just to make them easier to deploy.
You are right that 389-ds should not increase its complexity because of specific use. This is exactly the reason why I proposed that generic plugin. It is simple and is flexible enough to bring complexity outside of 389-ds. For example complex rewriter could be implemented/delivered by others projects (like freeipa) without the need to change 389-ds.

Look, I'm opposed to a generic rewrite framework - you either make a plugin that "rewrites" just what you need in a targeted manner, and it's simply "enabled or disabled", or you move this generic rewrite framework to freeipa because I really don't want it here. There is a better solution on the table for this project.

We are already completely hand tied on the current slapi plugin api because it's a generic framework, I don't think that we want to be on the hook for more.

Sorry to be so firm on this, we need simple solutions, not complex ones.

@firstyear I agree on this with you.
@tbordaz as my proposal initially was, just make support for these two rewrite plugins that serve AD-specific need. Sure, they can be in the same plugin because they are going to be used at the same time (and unlikely to be used separate).

@firstyear , @abbra thanks for you review and feedback. It looks there is an agreement to move rewriters/computed on projects that do need them (e.g. freeipa). I will close this PR, make the design and move the PR to freeipa project.

Mate, that's not what I'm saying at all. I want this code here in 389 instead of freeipa. Maybe I shouldn't have been so harsh. THis project will give you much better testing frameworks and reviews to slapi plugins.

What I'm saying is you need to simplify your design significantly. That's really what's going on.

@firstyear, at the moment the PR delivers a Generic plugin framework (to easily register filter/computed rewriter) and a specific rewrite for objectCategory.
I understand your concern about generic plugin, framework I will write a design about it as I think it is a valuable way to extend the usability of supported interfaces.

@abbra and yourself agreed that we should rather make things simpler and to locate the specific rewriter "its" specific plugin.

A question is where to develop the objectCategory specific rewriter plugin , in 389-ds or in freeipa. Others rewriter will be needed that will land into freeipa project because they are very specific or having depencies. At the moment I think it make more sense to move all AD-specific rewriters to freeipa.

Design for the generic plugin is http://www.port389.org/docs/389ds/design/search_rewriters.html

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3992

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.