integrating pdf generation kotlin files into api by yonglid · Pull Request #20 · zerobase-io/smart-tracing-api

yonglid · 2020-03-28T18:52:22Z

Generates QR codes and places them in PDF for orgs and sites.

Closes #21

toadzky

if you have questions about how to fix any of my comments, let me know

toadzky · 2020-03-28T21:10:06Z

+        </dependency>
+        <dependency>
+            <groupId>org.xhtmlrenderer</groupId>
+            <artifactId>flying-saucer-pdf-itext5</artifactId>


if possible, we should use the itext2 module. itext5 has licensing issues, so unless we need it, i'd avoid it.

thanks for bringing this up David! I saw that itext2 had some unfixed security bugs and is no longer maintained and flying saucer, updated 2 years ago,(https://github.com/flyingsaucerproject/flyingsaucer) recommended avoiding it. Also this thread went into details a bit more: flyingsaucerproject/flyingsaucer#123.
The bug is related to xml external entity attacks: https://seclists.org/bugtraq/2017/Nov/20.

It doesn't seem like it would matter in our use case since we're not parsing pdfs (javamelody/javamelody#780) but I decided to just use flying-saucer-openpdf based on recommendations, it meets our needs for now, and seems well maintained.

yonglid

Finished going through the pr. Addressed most comments. Left 3-4 comments to act on

yonglid · 2020-04-02T12:39:50Z

+
+        val renderedHtmlContent = templateEngine.process("template", context)
+        val xHtml = convertToXhtml(renderedHtmlContent)
+        val renderer = ITextRenderer()


I think this could be reusable since template won't change for a batch and the html will just reference the qr image. The qr image is mainly the thing that will change in a batch. What do you mean when you say save it as a field?

mbuchel · 2020-04-02T13:15:34Z

+
+        val renderedHtmlContent = templateEngine.process("template", context)
+        val xHtml = convertToXhtml(renderedHtmlContent)
+        val renderer = ITextRenderer()


make it a class member object, kindof like your logger

…censing reasons, cleaned up pdf generator replacing unnecessary annotations for exceptions with try catch and using java standard charset utf8 for emails and web page etc

…ompanion object; also generating pdfs outside of src now

…d manually handling closing an output stream etc

…y catch

… of the code base

toadzky · 2020-04-02T16:01:17Z

+ * /test.pdf
+ */
+class PdfGenerator {
+    private val logger: org.slf4j.Logger = LoggerFactory.getLogger(this.javaClass)


nit: make it a static and use the helper:

companion object { private val log by LoggerDelegate() }```

toadzky · 2020-04-02T16:02:51Z

+        try {
+            genQR.generateQRCodeImage(QR_VALUE, QR_WIDTH, QR_HEIGHT)
+        } catch (e: WriterException) {
+            logger.info("Could not generate QR Code, WriterException :: " + e.message)


for logging, don't append the message like this because a) you lose the stack trace and b) it's inefficient since it will always perform the string concatenation regardless of log level settings. the correct way to do this is:

log.info("log message", e)

toadzky · 2020-04-02T16:07:30Z

+        val templateResolver = ClassLoaderTemplateResolver()
+        templateResolver.prefix = "/pdfconfig/"
+        templateResolver.suffix = ".html"
+        templateResolver.templateMode = TemplateMode.HTML
+        templateResolver.characterEncoding = UTF_8
+        val templateEngine = TemplateEngine()
+        templateEngine.setTemplateResolver(templateResolver)


it would best to pass in a template engine here instead of creating it. we certainly don't need to create it each time we render a template

toadzky · 2020-04-02T16:08:38Z

+
+        val data = organizationData(businessname, town, state)
+        val context = Context()
+        val dataMap = HashMap<String, Any>()


fyi, a more idiomatic kotlin way would be

val dataMap = mapOf("data" to data, "logoPath" to QR_CODE_IMAGE_PATH)

toadzky · 2020-04-02T16:14:18Z

+
+        val renderedHtmlContent = templateEngine.process("template", context)
+        val xHtml = convertToXhtml(renderedHtmlContent)
+        val renderer = ITextRenderer()


it doesn't look like it's reusable since you are setting the document to the rendered content. leave it as is

"field" is a software term meaning "variable attached to an instance of a class", also known as a "member variable". how it's currently defined is a local variable, meaning it only exists during the execution of the method. a field, on the other hand, would be available in every method for the instance and only be created/set as needed instead of every time it's used. an example of a good field in this class is the template engine. we are always using the same engine with the same resolver using the same base path. recreating those each time is inefficient, both because it creates garbage that needs to be collected and because it can't cache the templates so it has the parse them anew each time.

toadzky · 2020-04-02T16:19:59Z

+
+class QRCodeGenerator(var logoPath: String, // have to use source path
+                     var qrCodeFinalPath: String) {
+    private val logger: org.slf4j.Logger = LoggerFactory.getLogger(this.javaClass)


same comment on logging as above

toadzky · 2020-04-02T16:23:43Z

+class QRCodeGenerator(var logoPath: String, // have to use source path
+                     var qrCodeFinalPath: String) {


it would be better if these were typed. if they are supposed to be file paths, use Path. if it's possible one of them is a resource instead of a file, you could use URL instead. stringly-typed apis are difficult to use correctly because there's no information on what the value should be.

general rule of thumb in a typed language: if you have a type or unit suffix on a primitive variable (string, number), you should probably be using a class that provides that information implicitly

toadzky · 2020-04-02T16:24:24Z

+        try {
+            ImageIO.write(combined, "png", outputfile)
+        } catch (e: IOException) {
+            throw Exception("image io write exception: " + e.message)


if you aren't going to do anything more than this, don't bother catching it

toadzky · 2020-04-02T16:24:49Z

+            g.dispose()
+            return imageBuff
+        } catch (e: IOException) {
+            throw Exception("scaleOverlay io exception: " + e.message)


same comment. catch blocks are for handling errors. if you aren't handling it, don't catch it

yonglid closed this Mar 28, 2020

yonglid reopened this Mar 28, 2020

yonglid requested review from mbuchel and toadzky March 28, 2020 19:11

yonglid linked an issue Mar 28, 2020 that may be closed by this pull request

QR / PDF Generation #21

Closed

toadzky requested changes Mar 28, 2020

View reviewed changes

mbuchel requested changes Mar 30, 2020

View reviewed changes

yonglid commented Apr 2, 2020

View reviewed changes

yonglid requested review from mbuchel and toadzky April 2, 2020 12:40

mbuchel requested changes Apr 2, 2020

View reviewed changes

yonglid and others added 13 commits April 2, 2020 10:45

integrating pdf generation

f46e668

deleted java version

58edc4d

adding more about pdf generation to readme

250b203

stylistic changes

ac49682

pdf changes

1eef67c

cleaned up pom xml / maven build, replaced itext5 with openpdf for li…

3d2ec70

…censing reasons, cleaned up pdf generator replacing unnecessary annotations for exceptions with try catch and using java standard charset utf8 for emails and web page etc

passing in output file path into generatePdf instead of defining in c…

3ffe87c

…ompanion object; also generating pdfs outside of src now

using kotlin's use to handle output stream while creating pdf to avoi…

1df670b

…d manually handling closing an output stream etc

fixing paths to account for compiling html and templates within JAR

86fc39c

returning byte array for email purposes; kept creating pdf functionality

3d2561d

Changing config db url back to localhost from default ip

9564253

replacing exception annotations in generate qr code generator with tr…

5fa27b5

…y catch

added logging instead of console out

d6a9c51

toadzky force-pushed the pdf-gen branch from d4ae883 to 85d8b38 Compare April 2, 2020 23:13

rebase onto master and integrate qr-code/pdf-generation with the rest…

85d8b38

… of the code base

toadzky approved these changes Apr 2, 2020

View reviewed changes

toadzky merged commit 4647359 into master Apr 2, 2020

toadzky deleted the pdf-gen branch April 2, 2020 23:28

yonglid restored the pdf-gen branch April 4, 2020 09:39

yonglid deleted the pdf-gen branch April 4, 2020 09:40

		class QRCodeGenerator(var logoPath: String, // have to use source path
		var qrCodeFinalPath: String) {

Conversation

yonglid commented Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toadzky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yonglid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yonglid commented Mar 28, 2020 •

edited

Loading