CCS-3388 Preprocessing the adoc to create an xref using uuid by ankkit24 · Pull Request #248 · redhataccess/pantheon

ankkit24 · 2020-03-06T20:25:39Z

No description provided.

…o CCS-3262

…o spike

benradey

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.

benradey · 2020-03-06T21:15:19Z

+                html = asciidoctor.convert(
+                        ascContent,
+                        OptionsBuilder.options().toFile(false));
+
+                log.info("asciidoctor content: {}", html);


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.

benradey · 2020-03-06T21:28:44Z

+        String uuid, newLink;
+
+        for (String line: lines) {
+            if(line.startsWith("xref:")){


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

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.

benradey · 2020-03-06T21:32:59Z

+                split = line.split(",pantheon-id=");
+                uuid = split[1].replace(split[1].substring(split[1].length()-1), "");                


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.

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.

benradey · 2020-03-06T21:34:52Z

+                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+"[");                


OOF, regex syntax!

I think I can read this. It mostly makes sense, but what is the purpose of the ? in the expression?

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 >>"

Ahhhh, the "?" makes it non-greedy! Ok, that makes sense, thank you very much!

benradey · 2020-03-06T21:39:17Z

+        JcrQueryHelper qh = new JcrQueryHelper(resolver);        
+        try {
+                Optional<Resource> result = 
+                    qh.query("select * from [nt:base] WHERE [jcr:uuid] = '" + uuid + "'")


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.

This makes sense, since this is searching for all the UUIDs across the hierarchy, we might have to switch to [pant:module]

benradey · 2020-03-06T21:41:32Z

+    private static Resource module;
+    private static final Logger log = LoggerFactory.getLogger(UuidPreProcessor.class);
+    private static String newModulePath;


These fields cannot be static. I'm happy to explain why, if you'd like me to.

benradey · 2020-03-06T21:45:07Z

+                resolveActualPath(module.getResourceResolver(), uuid);
+                newLink = split[0].replaceAll(":.*?\\[", ":"+newModulePath+"[");                
+                newLink = newLink + "]";
+                newLines.add(newLink);


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.

benradey · 2020-03-06T21:46:28Z

        Optional<Content> content;
        if (draft) {
            content = module.getDraftContent(LocaleUtils.toLocale(locale));
+            log.info("asciidoctorservlet content");


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.

I might have to delete this

benradey · 2020-03-06T21:47:42Z

    }

-    @Test
-    void dereferenceAllHyperlinks() {


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!

Yes Ben,
Once we have finalized and covered the cases in the code, I would write corresponding tests for them.

codecov-commenter · 2026-04-01T11:13:54Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 43.58974% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.59%. Comparing base (5111c56) to head (e06360f).
⚠️ Report is 336 commits behind head on master.

Files with missing lines	Patch %	Lines
...ntheon/asciidoctor/extension/UuidPreProcessor.java	33.33%	21 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #248      +/-   ##
============================================
- Coverage     63.13%   62.59%   -0.55%     
+ Complexity      295      290       -5     
============================================
  Files            74       75       +1     
  Lines          2452     2470      +18     
  Branches        388      390       +2     
============================================
- Hits           1548     1546       -2     
- Misses          840      859      +19     
- Partials         64       65       +1

Flag	Coverage Δ
java	`62.59% <43.58%> (-0.55%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ankit.n24 added 13 commits January 7, 2020 11:32

add: search page changes

c4b2f6a

Merge remote-tracking branch 'upstream/master' into CCS-3262

16371b3

Add: published and draft uploaded dates populated

da6a934

Date format changes

e7eb7cd

Update: date format update

34241b8

Update: review changes

8066523

Merge branch 'master' of https://github.com/redhataccess/pantheon int…

d48b0c5

…o CCS-3262

initial commit

2443f6e

Merge branch 'master' of https://github.com/redhataccess/pantheon int…

2c4d674

…o spike

update: format updated for asciidoc

d468249

changes to the uuid preprocessor as per the new inline attributes

9909af1

Update: referenced module path link obtained from uuid

1ce3b58

resolved conflicts

e06360f

ankkit24 requested a review from a team March 6, 2020 20:34

benradey suggested changes Mar 6, 2020

View reviewed changes

		split = line.split(",pantheon-id=");
		uuid = split[1].replace(split[1].substring(split[1].length()-1), "");

Uh oh!

Conversation

ankkit24 commented Mar 6, 2020

Uh oh!

benradey left a comment

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!

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!

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!

codecov-commenter commented Apr 1, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants