-
Story
-
Resolution: Done
-
Major
-
2.1.0.Final
-
None
We need an API to store the following Metadata.
<ruleset id=”WLS_10_12_TO_EAP_6” phase=”...”>
<metadata>
<dependencies>
<addon id=”org.jboss.windup.rules:windup-rules-javaee:2.0.1.Final”> <addon id=”org.jboss.windup.rules:windup-rules-java:2.0.0.Final”>
<!-- Do we need a specific version? Perhaps minimal?
What if an old rule runs in new windup which needs newer addon? etc.-->
</dependencies>
<sourcePlatform id=”weblogic” versionRange=”(10,12]”/>
<sourceFramework id=”ejb2”/>
<targetPlatform id=”eap6”/>
<targetFramework id=”ejb2”/>
<targetFramework id=”ejb3”/>
<tags>
<tag>require-stateless</tag>
<tag>require-nofilesystem-io</tag>
</tags>
<executeAfter></executeAfter>
<executeBefore></executeBefore>
</metadata>
</ruleset>
Currently we use Context hashmap to store the metadata.
We started using metadata for more that what Context was initially intended for, and storing to hashmap is becoming cumbersome and impractical (needs type checks and type casts; the map can store anything without type safety / validation).
For example:
If we have "tags" as a set of strings, but want to allow the user to pass a single string, or an array of strings, or a collection of strings, we need to do this:
private static void normalizeRuleMetadata(Rule rule) { if (!(rule instanceof Context)) return; Context ctx = (Context) rule; Object category = ctx.get(RuleMetadata.CATEGORY); if (category == null) { ctx.put(RuleMetadata.CATEGORY, new HashSet()); return; } if (category instanceof String) category = ((String)category).split(WindupRuleProviderBuilder.TAGS_SPLIT_PATTERN); if (category instanceof String[]) category = Arrays.asList((String[]) category); if (category instanceof Collection && !(category instanceof Set)) { HashSet categoriesSet = new HashSet(); for (Object catItem : (Collection) category) if (!(catItem instanceof String)) throw new WindupException("Rule categories may only contain strings, but contains: " + catItem.getClass().getSimpleName()); else if ("".equals(catItem)) continue; else categoriesSet.add(((String)catItem).trim()); ctx.put(RuleMetadata.CATEGORY, categoriesSet); }
The proper way (IMO) is to have several overloaded Java methods in the metadata API, which would do this in the right place, not based on type checks ex-post. More - the ex-post solution is forcing duplication and is error prone, because between calling such "normalizing" method and the origination of metadata, other code may tamper these data and either needs to duplicate those checks, or may forget them and fail on ClassCastException.
We should introduce a type-safe API which would allow the rule structures (Rule, RuleProvider, Ruleset) to have their metadata.
Lower level elements could query the parent elements for defaults or merge it's values with these, as appropriate for individual metadata types.
- is blocked by
-
-
- Closed
-
[WINDUP-520] Rule Externalization - Introduce a Rule, RuleProvider and RulesetMetadata API
Ondrej Zizka (Inactive)
added a comment - 1) WindupRuleMetadata should IMO be named RuleProvidersRegistry.
2) WindupRuleMetadata has:
private final List<WindupRuleProvider> providers = new ArrayList<>(); private final IdentityHashMap<WindupRuleProvider, List<Rule>> providersToRules = new IdentityHashMap<>(); private Configuration configuration;
Isn't the first redundant? It can be acquired simply by providersToRules.getKeys().
And WRT configuration - that one only has
public List<Rule> getRules();
Is that another redundancy, since rules are in the map? It could be retrieved as merge of getValues().
Ondrej Zizka (Inactive)
added a comment - 1) WindupRuleMetadata should IMO be named RuleProvidersRegistry.
2) WindupRuleMetadata has:
private final List<WindupRuleProvider> providers = new ArrayList<>();
private final IdentityHashMap<WindupRuleProvider, List<Rule>> providersToRules = new IdentityHashMap<>();
private Configuration configuration;
Isn't the first redundant? It can be acquired simply by providersToRules.getKeys().
And WRT configuration - that one only has
public List<Rule> getRules();
Is that another redundancy, since rules are in the map? It could be retrieved as merge of getValues(). IMO no such abstraction is needed here. We have pretty clear idea of what metadata we need.
Here's an example of what's wrong with context:
Normal way:
Set<String> tags = provider.getMetadata().getTags(); // TODO Use when WINDUP-520 implemented.
Context way:
Set<String> tags = new HashSet<>(); Context ctx = new ContextBase(){}; provider.enhanceMetadata(ctx); Object value = ctx.get(RuleMetadata.CATEGORY); if (value instanceof Collection) { for( Object tag : (Collection)value ) { if (tag instanceof String) tags.add((String)tag); } }
I will happily learn if there's better way to handle this with Context.
Ondrej Zizka (Inactive)
added a comment - - edited IMO no such abstraction is needed here. We have pretty clear idea of what metadata we need.
Here's an example of what's wrong with context:
Normal way:
Set< String > tags = provider.getMetadata().getTags(); // TODO Use when WINDUP-520 implemented.
Context way:
Set< String > tags = new HashSet<>();
Context ctx = new ContextBase(){};
provider.enhanceMetadata(ctx);
Object value = ctx.get(RuleMetadata.CATEGORY);
if (value instanceof Collection)
{
for ( Object tag : (Collection)value )
{
if (tag instanceof String )
tags.add(( String )tag);
}
}
I will happily learn if there's better way to handle this with Context. 
Lincoln Baxter III (Inactive)
added a comment - You could say the same thing for any abstraction. IMO, unless we really need it to use Context, we should rely on normal Java classes.
Using Context suffers from the ailments we are seeing now - (needs type checks and type casts; the map can store anything without type safety / validation). By using Context for the underlying implementation, we would only sweep the dirt under a nice carpet 
Ondrej Zizka (Inactive)
added a comment - - edited IMO, unless we really need it to use Context, we should rely on normal Java classes.
Using Context suffers from the ailments we are seeing now - (needs type checks and type casts; the map can store anything without type safety / validation). By using Context for the underlying implementation, we would only sweep the dirt under a nice carpet 
PR merged, closing.