myfaces-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From d...@apache.org
Subject [myfaces-trinidad] 04/30: Checkpoint: fixed bogus version range bug
Date Sun, 17 Sep 2017 12:29:45 GMT
This is an automated email from the ASF dual-hosted git repository.

deki pushed a commit to branch andys-skin-pregen
in repository https://gitbox.apache.org/repos/asf/myfaces-trinidad.git

commit 93143ed8e0c6baaedfb30ed97d408457b74c59b3
Author: Andy Schwartz <andys@apache.org>
AuthorDate: Mon Mar 12 18:10:35 2012 +0000

    Checkpoint: fixed bogus version range bug
    
    During skin pregeneration, StableNameUtils.ApplicationNameExtractor was sometimes seeing
bogus version ranges - ie. we had cases where our matched version range had start version
> end version.
    
    This was due to incorrect behavior that I previously implemented in AgentAtRuleMatcher.getVersionsForApplication().
This method returned all versions for the application, whereas during name generation we only
want to consider versions/version ranges that contain the current agent's version.
    
    So, for example, given the following @agent rule:
    
      @agent ie (version:8), ie (version:7)  { ... }
    
    In cases where the actual agent version is 7, AgentAtRuleMatcher.getVersionsForApplication()
would return entries for both 7 and 8.  This led to the following sequence during stable name
generation.
    
    Initially version range:
    
    start: MIN_VERSION
    end: MAX_VERSION
    
    After adding agent version 8:
    
    start: 8
    end: 8
    
    After then adding agent version 7:
    
    start: 8
    end: 7    (since 7 < 8)
    
    To avoid this, I have broken out AgentAtRuleMatcher.getVersionsForApplication() into two
methods:
    
    1. getAllVersionsForApplication(Application agentApplication): Returns all versions (eg.
7 and 8 in the above example).  This is used by AgentVariantExtractor to determine which versions
to attempt to pregenerate.  (We want to pregenerate both 7 and 8, so need to return all versions.)
    
    2. getMatchingVersionsForApplication(Application agentApplication, Version agentVersion):
Only returns versions that match the specified agent version.  This is used by StableNameUtils.ApplicationNameExtractor
when producing style sheet file names.  (We only want to take matching versions into consideration
when producing file names, so only return 7.)
    
    I have more refactoring to do in this area, eg. I don't think we actually want to expose
these two accessors at all.  But wanted to check this in  now since this issue is subtly annoying.
---
 .../trinidadinternal/skin/AgentAtRuleMatcher.java  | 38 ++++++++++++++++++++--
 .../skin/pregen/variant/AgentVariantExtractor.java |  4 +--
 .../style/util/StableNameUtils.java                | 38 +++++++++++++++-------
 .../trinidadinternal/resource/LoggerBundle.xrts    |  2 ++
 4 files changed, 65 insertions(+), 17 deletions(-)

diff --git a/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/skin/AgentAtRuleMatcher.java
b/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/skin/AgentAtRuleMatcher.java
index 7fa7620..07e197e 100644
--- a/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/skin/AgentAtRuleMatcher.java
+++ b/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/skin/AgentAtRuleMatcher.java
@@ -223,7 +223,7 @@ public final class AgentAtRuleMatcher
    * Returns a non-null Collection of agent applications that are matched by 
    * this matcher.
    */
-  public Collection<TrinidadAgent.Application> getMatchingApplications()
+  public Collection<TrinidadAgent.Application> getAllApplications()
   {
     return new ArrayList<TrinidadAgent.Application>(_selectorAgents.keySet());
   }
@@ -235,7 +235,7 @@ public final class AgentAtRuleMatcher
    * @param application the agent application for which matching version ranges
    *   should be returned.
    */
-  public Collection<Range<Version>> getVersionsForApplication(TrinidadAgent.Application
application)
+  public Collection<Range<Version>> getAllVersionsForApplication(TrinidadAgent.Application
application)
   {
     Collection<Range<Version>> ranges = new ArrayList<Range<Version>>();
 
@@ -253,6 +253,38 @@ public final class AgentAtRuleMatcher
     return ranges;
   }
 
+  /**
+   * Returns a non-null collection of agent version ranges that
+   * a) are matched by this matcher, and...
+   * b) contain the specified agent version.
+   * 
+   * @param application the agent application for which matching version ranges
+   *   should be returned.
+   * @param agentVersion only ranges that contain this version will be matched.
+   */  
+  public Collection<Range<Version>> getMatchingVersionsForApplication(
+    TrinidadAgent.Application application,
+    Version                   agentVersion
+    )
+  {
+    Collection<Range<Version>> allVersions = getAllVersionsForApplication(application);
+    Collection<Range<Version>> matchingVersions = 
+      new ArrayList<Range<Version>>(allVersions.size());
+    
+    for (Range<Version> range : allVersions)
+    {
+      // todo: Range.contains()
+      if ((range.getStart().compareTo(agentVersion) <= 0) &&
+          (range.getEnd().compareTo(agentVersion) >= 0))
+      {
+        matchingVersions.add(range);
+      }
+    }
+    
+    return matchingVersions;
+  }
+      
+
   private static Range<Version> _getVersionRange(AgentMatcher agentMatcher)
   {
     if (agentMatcher instanceof Versioned)
@@ -544,7 +576,7 @@ public final class AgentAtRuleMatcher
   }
 
   /**
-   * Utility interface used by getVersionsForApplication() to extract
+   * Utility interface used by getAllVersionsForApplication() to extract
    * version info from agent matchers.
    */
   private interface Versioned
diff --git a/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/skin/pregen/variant/AgentVariantExtractor.java
b/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/skin/pregen/variant/AgentVariantExtractor.java
index 7e2cb96..b601a4a 100644
--- a/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/skin/pregen/variant/AgentVariantExtractor.java
+++ b/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/skin/pregen/variant/AgentVariantExtractor.java
@@ -125,7 +125,7 @@ final class AgentVariantExtractor implements SkinVariantExtractor<ApplicationAnd
     assert(agentMatcher != null);
     
     Collection<TrinidadAgent.Application> nodeApplications =
-      agentMatcher.getMatchingApplications();
+      agentMatcher.getAllApplications();
       
     for (TrinidadAgent.Application application : nodeApplications)
     {
@@ -134,7 +134,7 @@ final class AgentVariantExtractor implements SkinVariantExtractor<ApplicationAnd
       
       if (supported)
       {
-        Collection<Range<Version>> versionRanges = agentMatcher.getVersionsForApplication(application);
+        Collection<Range<Version>> versionRanges = agentMatcher.getAllVersionsForApplication(application);
         _addVersions(application, versionRanges);        
       }
 
diff --git a/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/style/util/StableNameUtils.java
b/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/style/util/StableNameUtils.java
index 5c80651..68c99be 100644
--- a/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/style/util/StableNameUtils.java
+++ b/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/style/util/StableNameUtils.java
@@ -28,6 +28,7 @@ import java.util.Iterator;
 import java.util.Locale;
 
 import org.apache.myfaces.trinidad.context.Version;
+import org.apache.myfaces.trinidad.logging.TrinidadLogger;
 import org.apache.myfaces.trinidadinternal.agent.TrinidadAgent.Application;
 import org.apache.myfaces.trinidadinternal.skin.AgentAtRuleMatcher;
 import org.apache.myfaces.trinidadinternal.style.StyleContext;
@@ -196,7 +197,7 @@ public final class StableNameUtils
     {
       return Arrays.<NamingStyleSheetVisitor>asList(
                new PlatformNameExtractor(),
-               new ApplicationNameExtractor(),
+               new ApplicationNameExtractor(_context.getAgent().getVersion()),
                new LocaleNameExtractor(),
                new DirectionNameExtractor(),
                new AccessibilityNameExtractor());
@@ -452,9 +453,12 @@ public final class StableNameUtils
   // NamingStyleSheetVisitor that extracts the agent application name
   private static class ApplicationNameExtractor extends CollectionNameExtractor<Application>
   {
-    public ApplicationNameExtractor()
+    public ApplicationNameExtractor(Version agentVersion)
     {
-      _versionRange = Range.of(Version.MIN_VERSION, Version.MAX_VERSION);
+      assert(agentVersion != null);
+
+      _agentVersion = agentVersion;
+      _matchedVersions = Range.of(Version.MIN_VERSION, Version.MAX_VERSION);
     }
 
     @Override
@@ -466,7 +470,7 @@ public final class StableNameUtils
         return Collections.emptySet();
       }
       
-      return agentMatcher.getMatchingApplications();
+      return agentMatcher.getAllApplications();
     }
 
     @Override
@@ -489,13 +493,18 @@ public final class StableNameUtils
       assert(agentMatcher != null);
       assert(agentApplication != null);
 
-      Collection<Range<Version>> versionRanges = agentMatcher.getVersionsForApplication(agentApplication);
-      Range.intersect(_versionRange, versionRanges);
+      Collection<Range<Version>> versionRanges =
+        agentMatcher.getMatchingVersionsForApplication(agentApplication, _agentVersion);
+      Range.intersect(_matchedVersions, versionRanges);
 
-      if (_versionRange.getStart().compareTo(_versionRange.getEnd()) > 0)
+      if (_matchedVersions.getStart().compareTo(_matchedVersions.getEnd()) > 0)
       {
-        // todo: doc (eg. 1.9.0 vs. 1.9)
-        _versionRange.setEnd(_versionRange.getStart());
+        throw new IllegalStateException(
+          _LOG.getMessage("ILLEGAL_SKIN_AGENT_VERSION_RANGE",
+                                        new Object[] {
+                                          agentApplication,
+                                          _matchedVersions.getStart(),
+                                          _matchedVersions.getEnd() }));
       }
     }
 
@@ -522,8 +531,8 @@ public final class StableNameUtils
   
     private void _appendVersionName(StringBuilder builder)
     {
-      Version startVersion = _versionRange.getStart();
-      Version endVersion = _versionRange.getEnd();
+      Version startVersion = _matchedVersions.getStart();
+      Version endVersion = _matchedVersions.getEnd();
       boolean startMin = startVersion.equals(Version.MIN_VERSION);
       boolean endMax = endVersion.equals(Version.MAX_VERSION);
       
@@ -553,7 +562,8 @@ public final class StableNameUtils
       }
     }
 
-    private Range<Version> _versionRange;
+    private final Version  _agentVersion;
+    private Range<Version> _matchedVersions;
   }
 
   // NamingStyleSheetVisitor that extracts the locale name.
@@ -691,4 +701,8 @@ public final class StableNameUtils
   }
   
   private static final char _SEPARATOR = '-';
+
+  private static final TrinidadLogger _LOG =
+    TrinidadLogger.createTrinidadLogger(StableNameUtils.class);  
+  
 }
diff --git a/trinidad-impl/src/main/xrts/org/apache/myfaces/trinidadinternal/resource/LoggerBundle.xrts
b/trinidad-impl/src/main/xrts/org/apache/myfaces/trinidadinternal/resource/LoggerBundle.xrts
index ea17fa0..ae59a3e 100644
--- a/trinidad-impl/src/main/xrts/org/apache/myfaces/trinidadinternal/resource/LoggerBundle.xrts
+++ b/trinidad-impl/src/main/xrts/org/apache/myfaces/trinidadinternal/resource/LoggerBundle.xrts
@@ -1188,4 +1188,6 @@ The skin {0} specified on the requestMap will be used even though the
consumer''
 
 <resource key="ILLEGAL_SYSTEM_PROPERTY_VALUE">{0} is not a valid value for system property
{1}.  Valid values are: {2}</resource>
 
+<resource key="ILLEGAL_SKIN_AGENT_VERSION_RANGE">Illegal version range detected for
agent {0}.  Version range start {1} is greater than end {2}.</resource>
+
 </resources>

-- 
To stop receiving notification emails like this one, please contact
"commits@myfaces.apache.org" <commits@myfaces.apache.org>.

Mime
View raw message