:ExcludeEmailFromExport setting no longer has an effect

[pameyer]

in docker-aio:

[ERROR] Failures: 
[ERROR]   DatasetsIT.testExcludeEmail:565 expected:<[[]]> but was:<[sammi@sample.com]>
[INFO] 
[ERROR] Tests run: 76, Failures: 1, Errors: 0, Skipped: 1

which would appear to break #3443.

Not fully clear on where the 1 skip is coming from, but that's a problem for another day.

IQSS jenkins appears to be showing same behavior https://build.hmdc.harvard.edu:8443/job/phoenix.dataverse.org-apitest-develop/.

[pdurbin]

@pameyer thanks for opening this issue. I tweaked the title a bit and I'll paste a screenshot of http://guides.dataverse.org/en/4.9.4/installation/config.html#excludeemailfromexport to explain what the :ExcludeEmailFromExport setting does:

Based on the timing I suspect pull request #5047 may have introduced this regression from a quick look at the code, I can't tell. I can definitely reproduce the regression on my laptop. Here's how it looks in Netbeans:

[pdurbin]

@qqmyers @pameyer and I talked this out at http://irclog.iq.harvard.edu/dataverse/2018-10-15#i_76022 and I just made pull request #5191 to get :ExcludeEmailFromExport working again. Or at least for the specific test that was failing to pass.

[pameyer]

@pdurbin thanks - looks good to me (Tests run: 76, Failures: 0, Errors: 0, Skipped: 1). Still not sure where the skip is coming from, but that's a different problem.

[pdurbin]

@qqmyers thanks for pull request #5192 which is a variation on my pull request #5191. I'm going to let someone with more Java experience than me decide which approach is better. The main thing is that we've identified a fix for the regression.

[scolapasta]

It seems a little strange to have a static reference to an EJB. (but it also looks as if that was there before and you are just setting it*). Not necessarily for this issue, but as tech debt cleanup, I wonder if Json Printer should be a service bean as well (and then could just inject the setting service).

(*) one issue is for example changes like this were a change could break things, if the service bean is not set; the other thing I'm not sure if is that ejbs are supposed to be part of a pool and reused. This one bean would effectively reduce the size of the pook by one?

[scolapasta]

@qqmyers would you mind opining on my comment above and if you have any bandwidth to work on this tech debt now?

[qqmyers]

@scolapasta - I don't know. The entire class is a bunch of static methods, one of which needs the exclude email setting, which it then sends as a parameter to DatasetFieldWalker (could this be an EJB instead?).

Based on that, it seems a bit odd to me to have JsonPrinter be an EJB, but DatasetFieldWalker is mostly static as well and the other options above in our discussion aren't perfect either. A simple improvement might be to just store the exclude email boolean value rather than the whole settingsService.

However, I see that api calls like "{id}/versions/{versionNumber}/metadata/{block}" and any call to JsonPrinter.json(DatasetVersion) use the same code but currently doesn't appear to set the settingsService, so their return values will change depending on whether its called before or after a ddi export has been made. Given that, perhaps making DatasetFieldWalker and EJB or setting a static exclude email boolean in it at start-up might be best. (I think we could just propagate the excludeemail value as a (an optional?) method param through the JsonPrinter methods. That affects about 3 methods in JsonPrinter and gets rid of all state, but if its a required parameter, there's the web of calling methods that would need to look up that setting (unless those api and internal calls should include emails... and can just send false...)

Guess this is why things get left alone :-)

(FWIW, this sounds most similar to how the Workflow system worked/works - originally the doiProvider setting was passed into the workflow context rather than bringing the workflow steps into the EJB context and I just made that mechanism configurable so steps can define what settings they need.)
(Also FWIW: I initially tried to allow workflow steps to include settingsService and didn't know enough EJB to get it to work, so if you want to go EJB here, I'm not sure I'm the best person to try it (may be simpler since it isn't connected to transactions, etc.)
I'm willing to switch to just storing the excludeemail setting or making that a method parameter if you want to go one of those routes... penance for having broken the working code...

[scolapasta]

In theory, we'd want the settings to be changeable without having to restart (that is one differences between settings and jvm options and why we generally use jvm options more for "configuration" options instead of "customization" options).

Changing to an ejb would mean changing all the static methods to non static, so that does seem like overkill for this fix / cleanup (since the static reference to the ejb existed before), but still seems to me the right approach.

The other "right" way would be to do as you say and pass the parameter along, but I fear that would be messy and not scalable, if/when other settings are needed (like what you ran into with workflows. (for example, when we do convert this to ejb, you would have to go back and remove all these methods and calls, since then the Json printer could just call the SettingsService directly.

So I guess we can live with this current approach until someone has the time / need to convert this to an EJB.

[kcondon]

@qqmyers This fails to build with complier errors:

[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /var/lib/jenkins/workspace/Dataverse_external_rfi/src/test/java/edu/harvard/iq/dataverse/util/json/JsonPrinterTest.java:[200,35] constructor JsonPrinter in class edu.harvard.iq.dataverse.util.json.JsonPrinter cannot be applied to given types;
required: no arguments
found: edu.harvard.iq.dataverse.settings.SettingsServiceBean
reason: actual and formal argument lists differ in length
[ERROR] /var/lib/jenkins/workspace/Dataverse_external_rfi/src/test/java/edu/harvard/iq/dataverse/util/json/JsonPrinterTest.java:[241,35] constructor JsonPrinter in class edu.harvard.iq.dataverse.util.json.JsonPrinter cannot be applied to given types;
required: no arguments
found: edu.harvard.iq.dataverse.util.json.JsonPrinterTest.MockSettingsSvc
reason: actual and formal argument lists differ in length
[INFO] 2 errors

[qqmyers]

@kcondon - OK, try that. I updated the tests and will try to at least compile the test classes locally to not waste your time (trying to run the tests gives me lots of errors locally so I don't yet have a good way to test unless I merge changes into the build from QDR which runs at least some tests as part of an auto-build).

[pdurbin]

@kcondon @qqmyers you (and everyone) can all observe if unit tests are passing or failing at https://travis-ci.org/IQSS/dataverse/pull_requests

@scolapasta or I should have caught this earlier. Sorry.

[qqmyers]

@pdurbin - umm, I knew that :-) (at least from the PR page itself) @kcondon - I'll try to remember not to commit before a second cup of coffee...

[kcondon]

@qqmyers This works for 2 of the 3 formats that export contact email: ddi, and json, but fails for OAI_ORE.

[qqmyers]

@kcondon - the OAI-ORE export doesn't use JsonPrinter for this. I can fix that so OAI-ORE supports the setting, but it's nominally a separate problem. Do you want it in this pull or a separate one?

[kcondon]

@qqmyers If it is simple enough just add here, thanks.

[qqmyers]

@kcondon - ready to go.