Uploaded image for project: 'Maven Shared Components'
  1. Maven Shared Components
  2. MSHARED-489

AbstractMavenReportRenderer#linkPatternedText ignores name if href is invalid

    Details

      Description

      Consider this input:

      {My Text, ftp://host/file.txt} or {My Text, http:/host/file.txt} (typo)
      

      The current output simply be ftp://host/file.txt or http:/host/file.txt but I would expect it to be My Text in both cases.

      The problem is burried in the current code:

      for ( Iterator<String> it = segments.iterator(); it.hasNext(); )
      {
          String name = it.next();
          String href = it.next();
      
          if ( href == null )
          {
              text( name );
          }
          else
          {
              if ( getValidHref( href ) != null )
              {
                  link( getValidHref( href ), name );
              }
              else
              {
                  text( href );
              }
          }
      }
      

      The invalid href would be printed out instead of name. I think the code should read:

      for ( Iterator<String> it = segments.iterator(); it.hasNext(); )
      {
          String name = it.next();
          String href = it.next();
      
          if ( getValidHref( href ) == null )
          {
              text( name );
          }
          else
          {
              link( getValidHref( href ), name );
          }
      }
      

      Produce link if href isn't null and is valid otherwise add name (text) only.

        Issue Links

          Activity

          Hide
          hboutemy Hervé Boutemy added a comment -

          I think this issue should not be described as code change, but as unit test: what you currently get and what you think would be better to get
          = please describe the issue, not a solution to a yet to understand issue

          Show
          hboutemy Hervé Boutemy added a comment - I think this issue should not be described as code change, but as unit test: what you currently get and what you think would be better to get = please describe the issue, not a solution to a yet to understand issue
          Hide
          michael-o Michael Osipov added a comment -

          You are right. I will go ahead and improve it.

          Show
          michael-o Michael Osipov added a comment - You are right. I will go ahead and improve it.
          Hide
          hboutemy Hervé Boutemy added a comment -

          now I understand, thanks

          silently swallowing the invalid href seems strange: I'd expect a warning
          then, if there is a warning, I'm ok to render the text instead of the invalid url: but even that is questionable

          why not link to the apparently invalid url? after all, not rendering the link is not intuitive

          Show
          hboutemy Hervé Boutemy added a comment - now I understand, thanks silently swallowing the invalid href seems strange: I'd expect a warning then, if there is a warning, I'm ok to render the text instead of the invalid url: but even that is questionable why not link to the apparently invalid url? after all, not rendering the link is not intuitive
          Hide
          michael-o Michael Osipov added a comment - - edited

          Probably but what do you gain from linking an invalid URL? The browser will complain. Unless there is a warning, no one will improve it anyway.

          The getValidHref was some very strange code:

          String hrefTmp;
          if ( !href.endsWith( "/" ) )
          {
            hrefTmp = href + "/index.html";
          }
          else
          {
            hrefTmp = href + "index.html";
          }
          

          Does this really mean if my URL is http://sdfasf/wow.html or ftp://sdfsdf/super.html a /index.html is added? I wouldn't expect any URL manipulation here. / => /index.html is a websever/mod_dir thing. Not our business.

          Show
          michael-o Michael Osipov added a comment - - edited Probably but what do you gain from linking an invalid URL? The browser will complain. Unless there is a warning, no one will improve it anyway. The getValidHref was some very strange code: String hrefTmp; if ( !href.endsWith( "/" ) ) { hrefTmp = href + "/index.html" ; } else { hrefTmp = href + "index.html" ; } Does this really mean if my URL is http://sdfasf/wow.html or ftp://sdfsdf/super.html a /index.html is added? I wouldn't expect any URL manipulation here. / => /index.html is a websever/ mod_dir thing. Not our business.
          Hide
          hboutemy Hervé Boutemy added a comment -

          we completely agree: in case of invalid url, not having a warning is more a problem than rendering the url instead of the text
          as I see it, rendering the url was one form of warning, since rendering the text is a lot less visible

          Does this really mean if my URL is http://sdfasf/wow.html or ftp://sdfsdf/super.html a /index.html is added?

          once again, don't ask but write a unit test to show what is curently the effective result, even if you don't understand why this was done like this
          if there is no explanation in code, I don't have it myself either
          And I know I refactored code a good number of years ago for this method, then perhaps you're asking because svn annotate tells I wrote the code, but IIRC, at that time, I was refactoring to make algorithms easier to understand, but my first objective was to not change any result, even if I didn't understand some choices on semantics

          Show
          hboutemy Hervé Boutemy added a comment - we completely agree: in case of invalid url, not having a warning is more a problem than rendering the url instead of the text as I see it, rendering the url was one form of warning, since rendering the text is a lot less visible Does this really mean if my URL is http://sdfasf/wow.html or ftp://sdfsdf/super.html a /index.html is added? once again, don't ask but write a unit test to show what is curently the effective result, even if you don't understand why this was done like this if there is no explanation in code, I don't have it myself either And I know I refactored code a good number of years ago for this method, then perhaps you're asking because svn annotate tells I wrote the code, but IIRC, at that time, I was refactoring to make algorithms easier to understand, but my first objective was to not change any result, even if I didn't understand some choices on semantics
          Hide
          michael-o Michael Osipov added a comment -

          Hervé, I want to address this for 3.0. The simple approach is to print the name/text if href is invalid and log it. Should I simply use SLF4J? There is no logger in the abstract class and I do not know how to do it the Plexus way.

          Show
          michael-o Michael Osipov added a comment - Hervé, I want to address this for 3.0. The simple approach is to print the name/text if href is invalid and log it. Should I simply use SLF4J? There is no logger in the abstract class and I do not know how to do it the Plexus way.
          Hide
          michael-o Michael Osipov added a comment -

          Implicitly fixed with r1781702. Links are now passed as-is.

          Show
          michael-o Michael Osipov added a comment - Implicitly fixed with r1781702 . Links are now passed as-is.

            People

            • Assignee:
              michael-o Michael Osipov
              Reporter:
              michael-o Michael Osipov
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development