You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@myfaces.apache.org by de...@apache.org on 2017/09/17 12:29:45 UTC
[myfaces-trinidad] 04/30: Checkpoint: fixed bogus version range bug
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 <an...@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" <co...@myfaces.apache.org>.