New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Schema Generation for nested yml configurations #1027
Changes from 42 commits
774506b
7051ca9
f1b2d30
17079c7
886bc5a
0271cd3
522f4e5
f264c70
076fe14
2bd7f9d
0da381b
3d45f6b
cd9a9dd
4bf306c
4657e0c
c734c0f
fe1ce60
8108038
38dad38
d89e24c
3c4510c
f079505
76c0932
623914f
13e893a
2ede6f6
ec07960
448c4a2
3165323
ad57f8d
14197bc
a338fbc
d0f9349
6c4cb3c
901ac01
4df5e1e
81b9a68
fd886c8
6c42055
b0b870b
fe7b0b8
dcd5f6f
122c8d9
e5bde57
b97f380
1e0a6a2
d0083d8
a030035
2b105a4
79892a2
51ae5fd
cee3cf6
1f72339
67c3f81
dff9199
5388f66
e992aee
0e9e3bd
5bacdd2
b07c22b
9bc089a
94e14d4
a96a4f7
802c9c9
d50ad6e
26341c4
3ce5e2b
b92032d
5e9ce85
a71b71e
ce565f2
cd01391
e50c94f
55e0216
80221ea
2ef14a3
53342bf
2403838
0395bea
30fdbf8
912b46d
603a577
d271135
896d2cb
ee1c946
6ab2a31
3e32f25
3be3926
e7b2a8f
314f1a7
b646a04
423ee6c
d5d10f4
1cdd881
1040a6a
a242048
0cfde4a
1aa84b1
1ff2aaf
15a7500
b468ee9
772445a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,6 +61,7 @@ public class Attribute<Owner, Type> { | |
private Setter<Owner, Type> setter; | ||
private Getter<Owner, Type> getter; | ||
private boolean secret; | ||
private boolean isJsonSchema; | ||
|
||
private boolean deprecated; | ||
|
||
|
@@ -111,6 +112,14 @@ public Class<? extends AccessRestriction>[] getRestrictions() { | |
return restrictions != null ? restrictions : EMPTY; | ||
} | ||
|
||
/** | ||
* Set jsonSchema is used to tell the describe function to call the describe structure | ||
* so that it supports and returns a nested structure | ||
*/ | ||
public void setJsonSchema(boolean jsonSchema) { | ||
isJsonSchema = jsonSchema; | ||
} | ||
|
||
public boolean isRestricted() { | ||
return restrictions != null && restrictions.length > 0; | ||
} | ||
|
@@ -225,18 +234,29 @@ public CNode describe(Owner instance, ConfigurationContext context) throws Confi | |
": No configurator found for type " + type); | ||
} | ||
try { | ||
Object o = getValue(instance); | ||
if (o == null) { | ||
return null; | ||
Object o; | ||
if(isJsonSchema) { | ||
o = getType(); | ||
sladyn98 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (o == null) { | ||
return null; | ||
} | ||
} else { | ||
o = getValue(instance); | ||
if (o == null) { | ||
return null; | ||
} | ||
} | ||
|
||
// In Export we sensitive only those values which do not get rendered as secrets | ||
boolean shouldBeMasked = isSecret(instance); | ||
if (multiple) { | ||
Sequence seq = new Sequence(); | ||
if (o.getClass().isArray()) o = Arrays.asList((Object[]) o); | ||
for (Object value : (Iterable) o) { | ||
seq.add(_describe(c, context, value, shouldBeMasked)); | ||
if(o instanceof Iterable) { | ||
System.out.println("This is iterable"); | ||
for (Object value : (Iterable) o) { | ||
seq.add(_describe(c, context, value, shouldBeMasked)); | ||
} | ||
} | ||
return seq; | ||
} | ||
|
@@ -249,6 +269,51 @@ public CNode describe(Owner instance, ConfigurationContext context) throws Confi | |
} | ||
} | ||
|
||
|
||
/** | ||
* This function is for the JSONSchemaGeneration | ||
* @param instance | ||
* @param context | ||
* @return | ||
*/ | ||
timja marked this conversation as resolved.
Show resolved
Hide resolved
|
||
public CNode describeStructure(Owner instance, ConfigurationContext context) { | ||
|
||
sladyn98 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
final Configurator c = context.lookup(type); | ||
if (c == null) { | ||
return new Scalar("FAILED TO EXPORT\n" + instance.getClass().getName()+"#"+name + | ||
": No configurator found for type " + type); | ||
} | ||
try { | ||
Object o = new Object(); | ||
if(isJsonSchema) { | ||
sladyn98 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
o = getType(); | ||
if (o == null) { | ||
return null; | ||
} | ||
} | ||
|
||
// In Export we sensitive only those values which do not get rendered as secrets | ||
boolean shouldBeMasked = isSecret(instance); | ||
if (multiple) { | ||
Sequence seq = new Sequence(); | ||
if (o.getClass().isArray()) o = Arrays.asList((Object[]) o); | ||
if(o instanceof Iterable) { | ||
sladyn98 marked this conversation as resolved.
Show resolved
Hide resolved
sladyn98 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for (Object value : (Iterable) o) { | ||
seq.add(_describe(c, context, value, shouldBeMasked)); | ||
} | ||
} | ||
return seq; | ||
} | ||
return _describe(c, context, o, shouldBeMasked); | ||
} catch (Exception | /* Jenkins.getDescriptorOrDie */AssertionError e) { | ||
sladyn98 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Don't fail the whole export, prefer logging this error | ||
LOGGER.log(Level.WARNING, "Failed to export", e); | ||
return new Scalar("FAILED TO EXPORT\n" + instance.getClass().getName() + "#" + name + ": " | ||
+ printThrowable(e)); | ||
} | ||
} | ||
|
||
|
||
/** | ||
* Describes a node. | ||
* @param c Configurator | ||
|
@@ -261,7 +326,12 @@ public CNode describe(Owner instance, ConfigurationContext context) throws Confi | |
*/ | ||
private CNode _describe(Configurator c, ConfigurationContext context, Object value, boolean shouldBeMasked) | ||
throws Exception { | ||
CNode node = c.describe(value, context); | ||
CNode node; | ||
if(isJsonSchema) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Casz Added this so that it would not throw any casting errors, since in the descirbe structure we were initally calling with value whereas now we needed the type. This reduces the castClassException There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this still required?
sladyn98 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
node = c.describeStructure(value, context); | ||
} else { | ||
node = c.describe(value, context); | ||
} | ||
if (shouldBeMasked && node instanceof Scalar) { | ||
((Scalar)node).sensitive(true); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,8 @@ public class ConfigurationContext implements ConfiguratorRegistry { | |
|
||
private transient final ConfiguratorRegistry registry; | ||
|
||
private String mode; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's this mode value for? needs docs and probably should be an enum if required There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its just a mode to tell if the context is jsonSchema and if it is then run the appropriate function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be an enum or a boolean really There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah will incorporate it in the next set of changes. |
||
|
||
public ConfigurationContext(ConfiguratorRegistry registry) { | ||
this.registry = registry; | ||
} | ||
|
@@ -62,6 +64,15 @@ public void setUnknown(Unknown unknown) { | |
this.unknown = unknown; | ||
} | ||
|
||
public String getMode() { | ||
return mode; | ||
} | ||
|
||
public void setMode(String mode) { | ||
this.mode = mode; | ||
} | ||
|
||
|
||
|
||
// --- delegate methods for ConfigurationContext | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ | |
import org.apache.commons.lang.StringUtils; | ||
import org.jenkinsci.Symbol; | ||
|
||
|
||
/** | ||
* Define a {@link Configurator} which handles a configuration element, identified by name. | ||
* @author <a href="mailto:nicolas.deloof@gmail.com">Nicolas De Loof</a> | ||
|
@@ -167,4 +168,20 @@ default CNode describe(T instance, ConfigurationContext context) throws Exceptio | |
return mapping; | ||
} | ||
|
||
@CheckForNull | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs Javadoc since it will be consumed by plugins. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah added javadoc. Confused about the |
||
default CNode describeStructure(T instance, ConfigurationContext context) | ||
timja marked this conversation as resolved.
Show resolved
Hide resolved
|
||
throws Exception { | ||
Mapping mapping = new Mapping(); | ||
for (Attribute attribute : getAttributes()) { | ||
if(context.getMode().equals("JSONSchema")) { | ||
sladyn98 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
attribute.setJsonSchema(true); | ||
} | ||
CNode value = attribute.describeStructure(instance, context); | ||
if (value != null) { | ||
mapping.put(attribute.getName(), attribute.getType().getSimpleName()); | ||
} | ||
} | ||
return mapping; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,11 @@ | |
import com.google.gson.GsonBuilder; | ||
import com.google.gson.JsonElement; | ||
import com.google.gson.JsonParser; | ||
import io.jenkins.plugins.casc.impl.DefaultConfiguratorRegistry; | ||
import io.jenkins.plugins.casc.impl.configurators.DataBoundConfigurator; | ||
import io.jenkins.plugins.casc.impl.configurators.HeteroDescribableConfigurator; | ||
import io.jenkins.plugins.casc.model.CNode; | ||
import io.jenkins.plugins.casc.model.Mapping; | ||
import java.util.ArrayList; | ||
import java.util.Iterator; | ||
import java.util.LinkedHashSet; | ||
|
@@ -36,14 +40,15 @@ public static JSONObject generateSchema() { | |
* Iterates over the base configurators and adds them to the schema. | ||
*/ | ||
JSONObject schemaConfiguratorObjects = new JSONObject(); | ||
ConfigurationAsCode configurationAsCode = ConfigurationAsCode.get(); | ||
for (Object configuratorObject : configurationAsCode.getConfigurators()) { | ||
ConfigurationAsCode configurationAsCodeObject = ConfigurationAsCode.get(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bad merge I think, revert? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I will just get this to its original state. |
||
for (Object configuratorObject : configurationAsCodeObject.getConfigurators()) { | ||
if (configuratorObject instanceof BaseConfigurator) { | ||
BaseConfigurator baseConfigurator = (BaseConfigurator) configuratorObject; | ||
List<Attribute> baseConfigAttributeList = baseConfigurator.getAttributes(); | ||
|
||
if (baseConfigAttributeList.size() == 0) { | ||
schemaConfiguratorObjects | ||
.put(((BaseConfigurator) configuratorObject).getTarget().getSimpleName().toLowerCase(), | ||
.put(baseConfigurator.getName().toLowerCase(), | ||
new JSONObject() | ||
.put("type", "object") | ||
.put("properties", new JSONObject())); | ||
|
@@ -132,6 +137,10 @@ private static JSONObject generateNonEnumAttributeObject(Attribute attribute) { | |
attributeType.put("type", "integer"); | ||
break; | ||
|
||
case "hudson.Secret": | ||
attributeType.put("type", "string"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to add a comment to the schema to indicate that this field is sensitive, but it is rather a follow-up enhancement |
||
break; | ||
|
||
case "java.lang.Long": | ||
timja marked this conversation as resolved.
Show resolved
Hide resolved
|
||
attributeType.put("type", "integer"); | ||
break; | ||
|
@@ -154,6 +163,7 @@ private static JSONObject generateRootConfiguratorObject() { | |
RootElementConfigurator rootElementConfigurator = i.next(); | ||
rootConfiguratorObject | ||
.put(rootElementConfigurator.getName(), new JSONObject().put("type", "object")); | ||
System.out.println("root Name" + rootElementConfigurator.getName()); | ||
} | ||
return rootConfiguratorObject; | ||
} | ||
|
@@ -187,5 +197,50 @@ private static void generateEnumAttributeSchema(JSONObject attributeSchemaTempla | |
.put("enum", new JSONArray(attributeList))); | ||
} | ||
} | ||
|
||
public static void rootConfigGeneration() throws Exception { | ||
DefaultConfiguratorRegistry registry = new DefaultConfiguratorRegistry(); | ||
final ConfigurationContext context = new ConfigurationContext(registry); | ||
context.setMode("JSONSchema"); | ||
for (RootElementConfigurator root : RootElementConfigurator.all()) { | ||
final CNode config = root.describeStructure(root.getTargetComponent(context), context); | ||
final Mapping mapping = config.asMapping(); | ||
final List<Map.Entry<String, CNode>> entries = new ArrayList<>(mapping.entrySet()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Casz
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @timja Any ideas on How We can an Iterable Node Property ? Because we are returned a CNode which we cannot iterate on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a failing test or point to one I should run please? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this what you're after?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really because we wont enter this if statement at all. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You’re going to need to indicate that it’s a sequence some how, where I pointed is where the information is lost There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean that we should be iterating over the attribute type and keep adding it to the mapping something like this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no but you need to indicate it's a sequence? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indicate what as a sequence ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we indicate the mapping as a sequence would it make it easier to iterate further? |
||
for (Map.Entry<String, CNode> entry : entries) { | ||
System.out.println(entry.getKey()); | ||
} | ||
System.out.println("End of an iteration of configurators"); | ||
} | ||
} | ||
|
||
public static void storeConfiguratorNames() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @timja I am not sure this is necessary anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove if not needed |
||
ConfigurationAsCode configurationAsCodeObject = ConfigurationAsCode.get(); | ||
for (Object configuratorObject : configurationAsCodeObject.getConfigurators()) { | ||
if (configuratorObject instanceof BaseConfigurator) { | ||
BaseConfigurator baseConfigurator = (BaseConfigurator) configuratorObject; | ||
List<Attribute> baseConfigAttributeList = baseConfigurator.getAttributes(); | ||
for (Attribute attribute : baseConfigAttributeList) { | ||
if (attribute.multiple) { | ||
System.out.println( | ||
"This is a multiple attribute " + attribute.getType() + " " + attribute | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @timja We need a way to get the most out of the multiple attributes. |
||
.getName()); | ||
} else { | ||
if (attribute.type.isEnum()) { | ||
System.out.println("This is an enumeration attribute: "); | ||
if (attribute.type.getEnumConstants().length != 0) { | ||
System.out.println("Printing Enumeration constants for: " + attribute.getName()); | ||
for (Object obj : attribute.type.getEnumConstants()) { | ||
System.out.println("EConstant : " + obj.toString()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @timja The Enumeration constants are working fine: |
||
} | ||
} | ||
} | ||
} | ||
} | ||
} else if (configuratorObject instanceof HeteroDescribableConfigurator) { | ||
System.out.println("Instance of HeteroDescribable Configurator"); | ||
} | ||
} | ||
} | ||
|
||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,6 +117,7 @@ public CNode describe(T instance, ConfigurationContext context) { | |
}).getOrNull(); | ||
} | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. revert |
||
@SuppressWarnings("unused") | ||
public Map<String, Class<T>> getImplementors() { | ||
return getDescriptors() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Javadoc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added +1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for closing now it is the separate thread