Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace static calls with @Inject-able classes #7649

Closed
qqmyers opened this issue Mar 3, 2021 · 4 comments · Fixed by #7657
Closed

Replace static calls with @Inject-able classes #7649

qqmyers opened this issue Mar 3, 2021 · 4 comments · Fixed by #7657
Labels
GDCC: SciencesPO related to GDCC work for Sciences PO

Comments

@qqmyers
Copy link
Member

qqmyers commented Mar 3, 2021

There are a number of classes in Dataverse software that consist of static methods (JsonPrinter, MailUtil, and BrandingUtil in particular) where work-arounds have been made to allow settings to be accessed by the class (e.g. allowing JsonPrinter to use the :ExcludeEmailFromExport) or avoid using other services (e.g. sending the root dataverse collection name into BrandingUtil and MailUtil methods as a parameter, or finding the root name by following the chain of owners in DatasetVersion and JsonPrinter - versus calling dataverseService.findRootDataverse() as in other places.)

In addition to these current cases, the work proposed in #7387 will involve accessing settings in JsonPrinter and there is similar work @ QDR to use an 'institution name' instead of the root collection name in some cases that has avoided the issue by storing the institution name in the Bundle.properties file instead of as a setting.

After discussion with @scolapasta, rather than continuing to work around this issue, I've started working to replace the classes containing static methods with an @stateless class with non-static methods that can then be @injected where needed. Since this involves fairly straight forward changes across lots of classes, I'm going ahead to create a separate issue/PR. Nominally this PR should not change outward functionality - it does ~code cleanup and prep work for the work in #7387 (which I hope can then be reused by QDR to reduce the custom code in this area).

From some web searching, the general idea of replacing static methods with @stateless beans seems to be a reasonable/good practice. If anyone know of a reason we shouldn't do this in the Dataverse software, let me know. Also - if there are other cases where the same refactoring should be done - let me know - I can either look into doing those or at least adding some ToDo notes pointing to the classes above as examples.

@qqmyers qqmyers added the GDCC: SciencesPO related to GDCC work for Sciences PO label Mar 3, 2021
@poikilotherm
Copy link
Contributor

poikilotherm commented Mar 4, 2021

Hey @qqmyers,

I stumbled over some of these earlier this year, too, while looking at refactoring MailServiceBean for #7424. Talked to @pdurbin about those and my idea to get rid of some of this by using MicroProfile Config API. We stopped there to make MPCONFIG more approachable within the DV dev community (which resulted for example in the talk #7639 refers to)

While I generally believe in good use of the facade pattern, let me put some opinions here:

I definitely agree that we should refactor MailUtil and BrandingUtil to maybe just get rid of 'em (they are used in very limited spaces only).

For refactoring JsonPrinter, I find this very related to #6810 . IMHO we should talk about using MicroProfile Config to retrieve the data you mentioned where possible. And then start using JSON-B standard Jakarta EE tech for serialization in JSON instead of a selfbaked printer. IMHO that would remove a lot of boilerplate code, while still allowing to make custom serialization where necessary.

As always, I'm more than happy to join forces here. We can also do a call on this with @scolapasta 😄

@pdurbin
Copy link
Member

pdurbin commented Mar 4, 2021

All I ask is that code coverage does not go down. I created BrandingUtil, for example, and there are tests in BrandingUtilTest that should be preserved somewhere, please.

@qqmyers
Copy link
Member Author

qqmyers commented Mar 4, 2021

@poikilotherm Good ideas, but I'm not in a position to do the larger changes now. One question though - would the microprofile approach with the DBConfigSource you've created avoid having to change these classes from static methods to @stateless? I see the test where you just create a new DBConfigSource() (and then add a mock settingsservice to it. I don't see an example in the real code though - is it enough to just create a new DBConfigSource in, for example, BrandingUtil? Since you inject the SettingsServiceBean at startup, there's nothing else needed (aside from using DBConfigSource.getProperty()?)

@qqmyers
Copy link
Member Author

qqmyers commented Mar 8, 2021

Closing this in favor of #7657

@qqmyers qqmyers closed this as completed Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GDCC: SciencesPO related to GDCC work for Sciences PO
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants