sirix icon indicating copy to clipboard operation
sirix copied to clipboard

Clean architecture: Replace the `PageReadOnlyTrx` for serialization/deserialization of pages/nodes

Open JohannesLichtenberger opened this issue 2 years ago • 12 comments

It should not be given as a parameter to the serialize/deserialize methods to potentially read other data during serialization/deserialization of nodes for instance as it violates the layered architecture.

For instance, in the enum NodeKind the page read-only trx is used to read a name:

final String uri = pageReadTrx.getName(nameDel.getURIKey(), NodeKind.NAMESPACE);

In most occurrences the trx seems to be used to read the ResourceConfiguration, which might simply be given as a parameter.

JohannesLichtenberger avatar Aug 24 '22 21:08 JohannesLichtenberger

can i work on this issue

pernelkanic avatar Sep 25 '23 18:09 pernelkanic

@pernelkanic of course :) the enum in question is PageKind plus interface(s).

JohannesLichtenberger avatar Sep 25 '23 18:09 JohannesLichtenberger

In the PageKind I've removed the PageReadOnlyTrx parameter and added a ResourceConfiguration parameter for passing the necessary data required for serialization and deserialization? is that correct?

pernelkanic avatar Oct 05 '23 12:10 pernelkanic

Yes :-) maybe it would be even better to wrap the configuration in a context (simple data wrapper), but I guess it's ok :-)

JohannesLichtenberger avatar Oct 05 '23 18:10 JohannesLichtenberger

@pernelkanic can you open a PR?

JohannesLichtenberger avatar Oct 15 '23 18:10 JohannesLichtenberger

I have opened the PR, requesting review to know if there should be any changes.

pernelkanic avatar Oct 21 '23 04:10 pernelkanic

Currently it fails to compile, because I think an export missing.

JohannesLichtenberger avatar Oct 21 '23 13:10 JohannesLichtenberger

what should be done?

pernelkanic avatar Oct 24 '23 15:10 pernelkanic

It seems like org.jcp.xml.dsig.internal.dom.Utils is import that is causing the issue , it is considered internal and used in java.xml.crypto

pernelkanic avatar Oct 25 '23 14:10 pernelkanic

Does removing it fix the issue?

pernelkanic avatar Oct 25 '23 14:10 pernelkanic

Why do you need java.xml.crypto for the changes? Can you remove the import and update your PR?

JohannesLichtenberger avatar Oct 25 '23 15:10 JohannesLichtenberger

i have updated the pr

pernelkanic avatar Oct 25 '23 16:10 pernelkanic