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

Backward compatility issues with iText 2.1.7. #1085

Closed
RadekWikturna opened this issue Feb 28, 2024 · 23 comments · Fixed by #1086
Closed

Backward compatility issues with iText 2.1.7. #1085

RadekWikturna opened this issue Feb 28, 2024 · 23 comments · Fixed by #1086
Labels

Comments

@RadekWikturna
Copy link
Contributor

RadekWikturna commented Feb 28, 2024

This is not a real bug, but still, it's a serious issue for anyone trying to migrate from iText 2.1.7 to OpenPDF (currently 1.3.41).

OpenPDF, as a fork of iText, has the same classes, packages etc, which means the theoretically, all that is needed is to replace dependency on iText with OpenPDF. Theoretically. I'm just migrating our project and I had to adjust two classes, which is OK. We also have many other dependencies, one of them being Pentaho reports, which brings also iText 2.1.7 (damn!). So I excluded the iText libraries from Pentaho and checked that my project has no dependency on iText whatsoever. Then I tested printing some Pentaho reports, and to my surprise I got:
java.lang.NoSuchMethodError: com.lowagie.text.Cell.setVerticalAlignment(int)

signature of this method in iText:
public void setVerticalAlignment(int value)

signature of this method in OpenPDF:
public void setVerticalAlignment(final VerticalAlignment alignment)

I cannot change sources of a third-party library that depends on iText. Ideally, Pentaho should migrate to OpenPDF, but even the latest version still uses iText 2.1.7. I don't know if we can do anything about it.

But regardless of this situation with Pentaho, I was wondering if there are perhaps other methods which break compatibility with iText. I created a simple program that scans all public classes and their public fields, constructors and methods in a specifed package ("com.lowagie") and exports this to a file. I did this for iText 2.1.7 and for OpenPDF 1.3.41. The output is attached. You can then compare these 2 files in some tool that supports comparing text files (I used UltraEdit).

I found many changes. Some of these are new classes or new methods in OpenPDF, which is perfectly OK. Some are kind of 'false positives', e.g. when a method is no longer final, or when it throws more/less exceptions. But then there are many cases when a method signature has really changed or the method has been removed or renamed and this completely breaks backward compatibility!

Some examples:
public java.util.HashMap com.lowagie.text.Annotation.attributes() renamed to
public java.util.Map com.lowagie.text.Annotation.getAttributes()

public void com.lowagie.text.Cell.setHorizontalAlignment(int)
public void com.lowagie.text.Cell.setHorizontalAlignment(java.lang.String) replaced with
public void com.lowagie.text.Cell.setHorizontalAlignment(com.lowagie.text.alignment.HorizontalAlignment)

public java.util.HashMap com.lowagie.text.Chunk.getAttributes() renamed to
public java.util.Map com.lowagie.text.Chunk.getChunkAttributes()

public float com.lowagie.text.Paragraph.getSpacingAfter() removed
public float com.lowagie.text.Paragraph.getSpacingBefore() removed

in many cases an argument of type java.lang.Object has been change to a more specific type, e.g.
public boolean com.lowagie.text.SimpleCell.add(java.lang.Object) changed to
public boolean com.lowagie.text.SimpleCell.add(com.lowagie.text.Element)
I'm not sure if this is OK or not

in many cases an argument of specific type, e.g. ArrayList, changed to interface, e.g. List
I guess this is probably OK.

in many cases a specific return type, e.g. ArrayList, changed to interface, e.g. List
I guess this is probably OK.

To get an idea, compare the 2 files yourself.

Expected behavior
In the same major version, public API should be always backward compatible, methods should just marked deprecated. If this is not possible, then the major version has to increase (see semantic versioning). At least the 1.3 line should be backward compatible with iText 2.1.7, which should ease the migration, especially with third-party libraries.

This problem was addressed before (see #145) but definitely not thoroughly. There should be also some compatibility check done automatically (like I did by comparing method signatures). I don't know whether current state was introduced gradually during OpenPDF development or if it already existed (to some degree) in the original fork.

Screenshots
image

System (please complete the following information):

  • OS: Windows 11

OpenPdfClasses.txt
iTextClasses.txt

@Lonzak
Copy link
Contributor

Lonzak commented Feb 28, 2024

I agree. I think most of the methods have been thrown out in commit be38865 which was part of the modernization effort. However this has been moved to Version 2.0.0 and not 1.3.X. We should thus undo/restore the deletion of deprecated methods to maintain compatibility and keep iText as a drop-in-replacement (at least for 1.3.X)

@RadekWikturna
Copy link
Contributor Author

Wow, so it was only a very recent change...How come that nobody objected to such backward incompatible changes?!
I understand that having too many deprecated methods is not nice, but public API compatibility has higher priority, IMO. Such change should have gone only to the master branch (2.0), not to 1.3-java8 branch.

Can anyone revert it in the 1.3-java8, please?

@Lonzak
Copy link
Contributor

Lonzak commented Feb 28, 2024

There was a big discussion about the overall topic recently (#1053 if you are interested). We did focused on Java 8 compatibility and I didn't have the removed deprecations in mind. Can you add a pull request for this?

@RadekWikturna
Copy link
Contributor Author

Do you mean that I should somehow revert someone elses changes? Well, I don't feel like it. Can we ask @andreasrosdal to do that?

@Lonzak
Copy link
Contributor

Lonzak commented Feb 28, 2024

I reverted the changes + fixed changes from the last commit which broke java8 compatibility. See #1086

@RadekWikturna
Copy link
Contributor Author

RadekWikturna commented Feb 29, 2024

I recommend that Pentaho migrate to OpenPDF, instead of using a very old iText version.
That would be best. But that is out of our control, completely.

Regardless our problem with Pentaho, the point here is it should be possible to switch ANY project/library that currently uses iText 2.1.7 to OpenPdf 1.3.x just by changing the Gradle/Maven build file, i.e. no source code change should be necessary (and in case of third-party libraries this is not possible). That means that OpenPdf 1.3.x should be built with Java 8 (already done) and have the same method signatures as iText 2.1.7.
Of course, OpenPdf 2.0 (newer major version) can and should use newer Java and can be cleaned up by removing deprecated methods or having some breaking changes, as long as this is thoroughly described in a migration guide.

@asturio
Copy link
Member

asturio commented Feb 29, 2024

I'll try to keep this in 1.3. But some minor changes were still needed, even in 1.3. Many of the deprecation were introduced because of "bad" method signatures from iText and almost none usage of Generics.

  1. iText uses to many implementing classes in signatures, instead of their interfaces. So you have to many methods like this:
HashMap method(ArrayList list);

instead of

Map method(List list);
  1. No Generics, so casting things around is needed to much often. Collections are always of Object. Generics where introduced in java in 1.5 (2004-2009)

I don't know how good and complete the Migration-Guide is. But if you are migration your projects, it would be very nice if you could check that, and improve it, if needed (Wiki is open): https://github.com/LibrePDF/OpenPDF/wiki/Migrating-from-iText-2-and-4

@asturio
Copy link
Member

asturio commented Mar 4, 2024

1.3.42 was release. Please note that it can take some days until all repositories are updated. sonatype is already there:

https://central.sonatype.com/artifact/com.github.librepdf/openpdf/1.3.42

Can we close this issue?

@RadekWikturna
Copy link
Contributor Author

RadekWikturna commented Mar 5, 2024

I will retest once the 1.3.42 version is available on mvnrepository (currently not) and let you know.

@Lonzak
Copy link
Contributor

Lonzak commented Mar 5, 2024

@RadekWikturna Next time please test before - if something is still missing we could have added it to version 1.3.42. So you don't need to wait for 1.3.43 then...

@RadekWikturna
Copy link
Contributor Author

RadekWikturna commented Mar 5, 2024

I'm sorry, but how I can I test something if it's not released yet? Also, I've just noticed that the PR is merged (I don't look here every day and all the dozens of notifications just fall into a GitHub folder in my mailbox).

But I forgot to thank you for fixing it.

Edit: it has just appeared on mvnrepository. I'll test tomorrow, today I'm really busy.

@Lonzak
Copy link
Contributor

Lonzak commented Mar 5, 2024

I think we are all busy...

I'm sorry, but how I can I test something if it's not released yet?

You check out the branch (git clone it), build it (mvn clean build) and run it. Quite easy.

@RadekWikturna
Copy link
Contributor Author

I know that I can check out the project and build it. But what do yo mean by 'run it'? There's nothing to run. There's also no test that would discover incompatibility between iText (perhaps I will write one). So I need to test the real lib (from maven, fortunatelly it's available now) on our project and see if the exception doesn't occur any longer. I'm also again compare the method signatures.

@Lonzak
Copy link
Contributor

Lonzak commented Mar 5, 2024

By "run it" I meant 'use it or apply it' in whatever form. In your original post you said you tested a migration:
You replaced iText 2.1.7 with OpenPDF 1.3.41.
Now with the branch you have checked out, you would do a maven clean install which installs the dependency in your local repo. Then you can use this version in your migration effort (and not the one you did use).
=> So you would have replaced iText 2.1.7 with OpenPDF 1.3.42-SNAPSHOT.

PS: Thank you for testing now. This was just a hint for improving for the future...

@asturio
Copy link
Member

asturio commented Mar 5, 2024

Quite an interesting discussion here. I'll try to document how one can use a SNAPSHOT-Version, so one can test, if problems were fixed in the development-Branch.

@asturio
Copy link
Member

asturio commented Mar 5, 2024

Just created this Wiki Page:

https://github.com/LibrePDF/OpenPDF/wiki/Using-a-SNAPSHOT-Version

Reviews are welcome.

@RadekWikturna
Copy link
Contributor Author

Ok, today I've retestested the original problem - printing with Pentaho reporting engine, which normaly uses iText 2.1.7, but i replaced it in build file with openPdf 1.3.42.

The original exception doesn't occur. However, there's a new exception:

java.lang.NoSuchMethodError: com.lowagie.text.TextElementArray.add(Ljava/lang/Object;)Z, [eid: 1008284781]

          STACK TRACE:
          at org.pentaho.reporting.engine.classic.core.modules.output.table.rtf.helper.RTFTextExtractor$StyleContext.add(RTFTextExtractor.java:201)
          at org.pentaho.reporting.engine.classic.core.modules.output.table.rtf.helper.RTFTextExtractor.finishInlineBox(RTFTextExtractor.java:252)
          at org.pentaho.reporting.engine.classic.core.layout.process.IterateStructuralProcessStep.startProcessing(IterateStructuralProcessStep.java:124)
          at org.pentaho.reporting.engine.classic.core.modules.output.table.base.DefaultTextExtractor.processTextLine(DefaultTextExtractor.java:448)
          at org.pentaho.reporting.engine.classic.core.modules.output.table.base.DefaultTextExtractor.processParagraphChilds(DefaultTextExtractor.java:386)
          at org.pentaho.reporting.engine.classic.core.modules.output.table.rtf.helper.RTFTextExtractor.processParagraphChilds(RTFTextExtractor.java:366)
          at org.pentaho.reporting.engine.classic.core.layout.process.IterateStructuralProcessStep.startProcessing(IterateStructuralProcessStep.java:93)
          at org.pentaho.reporting.engine.classic.core.modules.output.table.base.DefaultTextExtractor.compute(DefaultTextExtractor.java:91)

iText 2.1.7 method signature:
public boolean add(Object o);

OpenPdf 1.3.42 method sifnature:
boolean add(Element o);

It seems that this change is not binary compatible.

Also in our project, which originally used iText 2.1.7 and now we are trying to switch to OpenPdf, I have a new compiler error (which didn't occur with OpenPdf 1.3.41):

Map interfaceProps = new HashMap();
List<Element> elements = HTMLWorker.parseToList(new StringReader(html), styleSheet, (HashMap)interfaceProps); // cast is needed because iText method signature requires a HashMap

there are now 2 methods in HTMLWorker and the compiler now complains:
compile error: The method parseToList(Reader, StyleSheet, HashMap) is ambiguous for the type HTMLWorker

    @Deprecated
    public static ArrayList<Element> parseToList(Reader reader, StyleSheet style, HashMap interfaceProps)

    public static ArrayList<Element> parseToList(Reader reader, StyleSheet style, Map<String, Object> interfaceProps)

Ok, I can change this compile error easily by removing the cast. But I'm not sure if this doubling of methods is binary compatible.

@RadekWikturna
Copy link
Contributor Author

attached are again public classes of iText and OpenPdf (1.3.41 and 1.3.42). Comparing these files (e.g. in UltraEdit or IDEA) shows the differences in method signatures. I can't tell which of these are really binary incompatible, I would say at least all those add(Object) that changed to add(Element) ans similar, but perhaps also those method(ArrayList) that changed to method(List) etc.

OpenPdf1_3_42_Classes.txt
iTextClasses.txt
OpenPdf1_3_41_Classes.txt

@asturio
Copy link
Member

asturio commented Mar 6, 2024

The way to go is to:

  1. Create new methods with a better signature.
  2. Keep the methods with the original signature, and mark these as deprecated, and use call there the newer method. So for compatibility the old method is still there, but one should use the new ones.

Changing the type of parameter will break the API. The new method may have better parameter types, and can be called the same.

Changing the return type of a method only is not possible. The new method must have a different name.

@asturio
Copy link
Member

asturio commented Mar 6, 2024

Question, should the original method be added again as deprecated, or is the overhead of updating the code manageable/doable?

@asturio
Copy link
Member

asturio commented Mar 6, 2024

    @Deprecated
    public static ArrayList<Element> parseToList(Reader reader, StyleSheet style, HashMap interfaceProps)

    public static ArrayList<Element> parseToList(Reader reader, StyleSheet style, Map<String, Object> interface

This is actually ok. If you call the method using a HashMap, the former one will be called, if you have a Map with generics, the later one. If you have a nul' then you MUST cast the null to HashMap or Map<>, to get rid of the compiler error. But your compiled code against 1.3.41 will call the old method in 1.3.42.

@asturio
Copy link
Member

asturio commented Mar 8, 2024

Hi @RadekWikturna ,

I just tried to add the add(Object) method back to the API, and that is not possible anymore. So you probably need to update your code, or recompile your code.

add(Object) was modernized in 2019 to add(Element), and the API is being so since 1.3.0. With 1.2.21 the API was still there.

I think OpenPDF is not a simple drop-in replacement for iText 2.1.7. One of the aspects of all devs was to try to modernize the API where possible. Using Generics is one of the goals. More Enums (instead of String or int constants), and so on. So there are here and there some breaking changes.

But moving to OpenPDF shouldn't be that difficult, if you have access to the code that uses it. If you are using iText indirectly through a 3rd party library, you can't change to OpenPDF that way. That library needs to be upgraded to use OpenPDF too.

@asturio
Copy link
Member

asturio commented Mar 27, 2024

I'm closing this one now, as the last removed deprecated code was put back in 1.3, and it is important to keep some API-compatibility inside one branch, like in the 1.3-line.

Please feel free opening a new Issue or PR, with some code to increase the compatibility to iText. Or add some examples of what is needed to be changed in the code of an existing project, that uses itext, to use OpenPDF.

@asturio asturio closed this as completed Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants