Bmoric/refacto secret processor

[benmoriceau]

What

Follow up on the comments on #11296

It is reactoring the JsonProcessor by making it being generated through a factory. This allow to avoid propagated the feature flag inside the JsonProcessor.

[michel-tricot]

Not what I had in mind. You still have complex logic.

What about instead you do the following:

when you instantiate the DatabaseConfigPersistence:
JsonSecretsProcessor jsp = flag ? new RealJsonSecretsProcessor() : new NoOpJsonSecretsProcessor();

new DatabaseConfigPersistence(jsp);

This way you don't create more complex logic in the config persistence, the testing of that class is simple, you don't need to pass a colloection for feature flags.

[benmoriceau]

I changed it to a class, I am not 100% satisfy with it because we don't really want a no op for the copy secrets step since it is not affected by the previous comments. I think that I should change it to a factory instead to generate the JsonSecretsProcessor that will do or not do an operation.

[michel-tricot]

The current state is definitely in the vein of what I had in mind.

[benmoriceau]

I changed it to a factory. I prefer this implementation because while it is allowing to have a single pace for implementation, it allows to have several type of combination, like copy but don't mask, mask and copy, etc ...

[edgao]

not sure I understand why these methods need to be overridden here. I.e. why not just move maskSecrets and copySecrets into JsonSecretsProcessor and make it a concrete class?

[benmoriceau]

It comes from a previous PR: #11296. In order to still be able to use the export functionality, we want to sometime not mask the secret. That's why we have this behavior of having a flag to mask or not mask the secrets.

[edgao]

I think my question is more about why JsonSecretsProcessor needs to be an abstract class, and why we need to create an anonymous inner class in the factory. I.e. could JsonSecretsProcessor look something like this:

// note the non-abstract class
public class JsonSecretsProcessor {
  private final boolean maskSecrets;
  private final boolean copySecrets;

  public JsonNode maskSecrets(final JsonNode obj, final JsonNode schema) {
    // do stuff...
  }

  public JsonNode copySecrets(final JsonNode src, final JsonNode dst, final JsonNode schema) {
    // do other stuff...
  }
}

and then JsonSecretsProcessorFactory#createJsonSecretsProcessor would just be return new JsonSecretsProcessor(maskSecrets, copySecrets);

[edgao]

nitpick: canBeProcessed is already checking that schema is an object, so this check is unnecessary

[benmoriceau]

Done

[edgao]

Suggested change

	// i.e: if the value of the secret isn't set to the mask
	// I.e. if the destination object's value is set to the mask, then we can copy the original secret

nitpick: match the comment ("copy if equal to mask") with what the branch is actually doing (if (dst.get(key) == MASK) { dst.set(src.get(key)) })

[benmoriceau]

Done

[edgao]

I think I wrote this else-case originally so this is a bit awkward, but this would be a more helpful comment (right?):

Suggested change

	// Otherwise, this is just a plain old json object; recurse into it.
	// Otherwise, this is just a plain old json node; recurse into it. If it's not actually an object, the recursive call will exit immediately.

[edgao]

nitpick: it seems a bit weird that maskSecrets doesn't actually mask secrets sometimes. Not sure what a better name would be though - maybe prepareSecretsForOutput or something? (I'm very bad at naming :P )

[benmoriceau]

That's true, I have updated it.

[edgao]

is this ever set to false in production? (and why is it necessary for tests?)

[edgao]

one last comment about the class structure; otherwise

[edgao]

I think my question is more about why JsonSecretsProcessor needs to be an abstract class, and why we need to create an anonymous inner class in the factory. I.e. could JsonSecretsProcessor look something like this:

// note the non-abstract class
public class JsonSecretsProcessor {
  private final boolean maskSecrets;
  private final boolean copySecrets;

  public JsonNode maskSecrets(final JsonNode obj, final JsonNode schema) {
    // do stuff...
  }

  public JsonNode copySecrets(final JsonNode src, final JsonNode dst, final JsonNode schema) {
    // do other stuff...
  }
}

and then JsonSecretsProcessorFactory#createJsonSecretsProcessor would just be return new JsonSecretsProcessor(maskSecrets, copySecrets);