CCS-3388 Preprocessing the adoc to create an xref using uuid #248
Conversation
There was a problem hiding this comment.
Ankit, this is a great start, but while I was reviewing it I realized that there are a number of cases that the code won't catch! It looks like for that reason, the preprocessor approach may wind up being more complicated than the postprocessor approach, but it's still worth investigating. Please see my code review comments and see if you can make the code cover those cases. If you have trouble, let's talk about it. As always, let me know if you have questions.
| html = asciidoctor.convert( | ||
| ascContent, | ||
| OptionsBuilder.options().toFile(false)); | ||
|
|
||
| log.info("asciidoctor content: {}", html); |
There was a problem hiding this comment.
Please remove these lines. You are calling .convert() twice here - the first result gets overwritten by the second, so it must be unnecessary. We probably don't need the log statement here either.
| String uuid, newLink; | ||
|
|
||
| for (String line: lines) { | ||
| if(line.startsWith("xref:")){ |
There was a problem hiding this comment.
This is a good start but it needs to be updated so that it works for all of these scenarios:
- Line has an "xref:" but it's not at the start of the line.
- Author uses the "<<xrefTarget,xrefLabel>>" syntax (I'm not sure how parameters are supplied in this scenario, if it's even possible... please find out)
- Author uses a "link:" instead of an xref
There was a problem hiding this comment.
Yes Ben,
These cases look good and there are a few more cases that crossed my mind.
I would want to incorporate as many possible in this scenario.
| split = line.split(",pantheon-id="); | ||
| uuid = split[1].replace(split[1].substring(split[1].length()-1), ""); |
There was a problem hiding this comment.
I'm not quite sure what this does. Are we assuming that the 'pantheon-id' parameter is the final parameter of the xref, and then the last character is the closing ] bracket, and then the line ends? Anyway, please make this a bit more reliable like we discussed in-person.
Don't spend too much time making the code update "perfect" - I'm not sure that we'll stick with UUIDs for the final solution, so just getting the next N characters (N = however long a UUID is) from the string would be good enough for this POC.
There was a problem hiding this comment.
This approach was done considering the last character to be ']' and assuming 'pantheon-id' as the final parameter of xref.
Since I would be working with more number of cases as mentioned in the above comments, I would probably applying some regex to identify and resolve the UUID.
| split = line.split(",pantheon-id="); | ||
| uuid = split[1].replace(split[1].substring(split[1].length()-1), ""); | ||
| resolveActualPath(module.getResourceResolver(), uuid); | ||
| newLink = split[0].replaceAll(":.*?\\[", ":"+newModulePath+"["); |
There was a problem hiding this comment.
OOF, regex syntax!
I think I can read this. It mostly makes sense, but what is the purpose of the ? in the expression?
There was a problem hiding this comment.
Explained the use of "?" in the below example:
input = "The << first match >> and << second match >> of the input string"
regex = <<.>> matches the whole string "<< first match >> and << second match >>"
But
regex = <<.?>> produces two matches: "<< first match >>" and "<< second match >>"
There was a problem hiding this comment.
Ahhhh, the "?" makes it non-greedy! Ok, that makes sense, thank you very much!
| JcrQueryHelper qh = new JcrQueryHelper(resolver); | ||
| try { | ||
| Optional<Resource> result = | ||
| qh.query("select * from [nt:base] WHERE [jcr:uuid] = '" + uuid + "'") |
There was a problem hiding this comment.
I know we talked about having [nt:base] here earlier. It may not make a difference from a performance perspective, but from a functionality perspective, it makes sense to put [pant:module] here instead, defensively. The reason is that we may expose UUIDs for other objects (maybe products, for some reason?) to authors at some point in the future. If the author accidentally supplies a product's UUID in an xref, we wouldn't want this code resolving that. It is better to specify that we are looking for modules only and to return no result in such a scenario.
There was a problem hiding this comment.
This makes sense, since this is searching for all the UUIDs across the hierarchy, we might have to switch to [pant:module]
| private static Resource module; | ||
| private static final Logger log = LoggerFactory.getLogger(UuidPreProcessor.class); | ||
| private static String newModulePath; |
There was a problem hiding this comment.
These fields cannot be static. I'm happy to explain why, if you'd like me to.
| resolveActualPath(module.getResourceResolver(), uuid); | ||
| newLink = split[0].replaceAll(":.*?\\[", ":"+newModulePath+"["); | ||
| newLink = newLink + "]"; | ||
| newLines.add(newLink); |
There was a problem hiding this comment.
I would like to see this logic being conditional based on whether or not the UUID query returns a result. It looks like the current behavior is that if the UUID fails to resolve, the xref is totally wiped out and replaced with nothing. What the code should do is leave the original author-supplied xref target intact.
| @@ -57,6 +57,7 @@ protected void doGet(SlingHttpServletRequest request, SlingHttpServletResponse r | |||
| Optional<Content> content; | |||
| if (draft) { | |||
| content = module.getDraftContent(LocaleUtils.toLocale(locale)); | |||
| log.info("asciidoctorservlet content"); | |||
There was a problem hiding this comment.
You shouldn't need to touch this file. I am 90% sure that this servlet is unused in our project... but no one has gotten around to deleting it.
There was a problem hiding this comment.
I might have to delete this
| @@ -68,44 +68,6 @@ void encodeAllImageLocations() { | |||
| }); | |||
| } | |||
|
|
|||
| @Test | |||
| void dereferenceAllHyperlinks() { | |||
There was a problem hiding this comment.
It definitely makes sense to remove this test since you're removing the code that it targets, but don't forget to add appropriate tests of your own for the new code that you're introducing!
There was a problem hiding this comment.
Yes Ben,
Once we have finalized and covered the cases in the code, I would write corresponding tests for them.
No description provided.