-
Notifications
You must be signed in to change notification settings - Fork 14
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
Support for json-schema in configuration source type #66
Support for json-schema in configuration source type #66
Conversation
709b2d4
to
543470a
Compare
result.write(buffer, 0, length); | ||
} | ||
gen.writeFieldName("schema"); //$NON-NLS-1$ | ||
gen.writeUTF8String(buffer, 0, buffer.length); |
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.
Alternatively, we could encode and send it as binary:
gen.writeFieldName("schema"); //$NON-NLS-1$
gen.writeBinary(Base64Variants.getDefaultVariant(), inputStream, -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.
Have you considered sending the schema as a JSON object instead? Then we avoid having to unquote and reparse it as JSON on the consumer side. It does mean that the server needs to read in the schema as a JSON object itself instead of just quoting it as a UTF8 string, but the schema will need to be read in anyway for the server to be able to enforce it. Potentially the same in-memory representation could be used. I expect the schema-files to be relatively small so I don't see it as a performance problem.
The "schema" key in the response would map to a JSON Object instead of a JSON string.
The XML analysis configuration source type response would then look like:
{
"id": "org.eclipse.tracecompass.tmf.core.config.xmlsourcetype",
"name": "XML Data-driven analyses",
"description": "Data-driven analyses described in XML",
"parameterDescriptors": [
{
"keyName": "path",
"dataType": "STRING",
"description": "URI to XML analysis file",
"isRequired": true
}
],
"schema": {
"$schema": "https://json-schema.org/draft/2020-12/schema",
"$id": "https://org.eclipse.tracecompass/custom-execution-analysis.json",
"title": "Custom Execution Analysis",
"description": "Custom Execution Analysis schema",
"type": "object",
"properties": {
"cpus": {
"description": "array of integer",
"type": "array",
"items": {
"type": "number"
}
},
"thread": {
"description": "Thread regular expression pattern",
"type": "string"
},
"phone": {
"description": "Phone number",
"type": "string",
"pattern": "^(\\([0-9]{3}\\))?[0-9]{3}-[0-9]{4}$"
},
"label": {
"description": "Optional label text",
"type": "string"
}
},
"required": [
"thread"
]
}
}
The serialization could look like this (unless there's already a parsed in-memory representation available):
diff --git a/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core/src/org/eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/webapp/TmfConfigurationSourceTypeSerializer.java b/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core/src/org/eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/webapp/TmfConfigurationSourceTypeSerializer.java
index 4eee84cb..35bcbd5a 100644
--- a/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core/src/org/eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/webapp/TmfConfigurationSourceTypeSerializer.java
+++ b/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core/src/org/eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/webapp/TmfConfigurationSourceTypeSerializer.java
@@ -11,7 +11,6 @@
package org.eclipse.tracecompass.incubator.internal.trace.server.jersey.rest.core.webapp;
-import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
@@ -20,6 +19,8 @@ import java.io.InputStream;
import org.eclipse.tracecompass.tmf.core.config.ITmfConfigurationSourceType;
import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.SerializerProvider;
import com.fasterxml.jackson.databind.ser.std.StdSerializer;
@@ -55,13 +56,10 @@ public class TmfConfigurationSourceTypeSerializer extends StdSerializer<ITmfConf
File schemaFile = value.getSchemaFile();
if (schemaFile != null) {
try (InputStream inputStream = new FileInputStream(schemaFile)) {
- ByteArrayOutputStream result = new ByteArrayOutputStream();
- byte[] buffer = new byte[1024];
- for (int length; (length = inputStream.read(buffer)) != -1; ) {
- result.write(buffer, 0, length);
- }
+ ObjectMapper mapper = new ObjectMapper();
+ JsonNode schema = mapper.readTree(inputStream);
gen.writeFieldName("schema"); //$NON-NLS-1$
- gen.writeUTF8String(buffer, 0, buffer.length);
+ gen.writeTree(schema);
}
}
gen.writeEndObject();
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.
Thanks for the feedback. Yes, I thought about it. But when I tried it I was not able to implement a proper deserializer in the test class TmfConfigurationSourceTypeStub
(see above). My first attempt for that was to deserialize it as an Object
(instead of String
) and the resulting object was a bunch of LinkedHashMaps. I didn't pursue it further. After your comment I looked into it again and found a way. I just store it as a JsonNode
object, which worked fine. So, it is a viable alternative.
Not sure how to document that element is a arbitrary json object in the swagger documentation (see first commit). Any ideas (@awendelin-work)?
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.
We have a similar challenge when creating a configuration instance using a json object (POST). We can serialize a json object and the service endpoint can deserialize it as JsonNode from com.fasterxml.jackson.databind
. However, then we need to pass it on to the trace compass core implementation. Right now the API asks for Map<String, Object>. We have multiple options:
- pass the
JsonNode
object. This will introduce a hard dependency tocom.fasterxml.jackson.databind
in Trace Compass core - pass it on as json string using
JsonNode.toString()
- save it to a temporary file and pass the file on
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.
Not sure how to document that element is a arbitrary json object in the swagger documentation (see first commit). Any ideas (@awendelin-work)?
Swagger treats a Java Object as a JSON Object, so it would be possible to annotate the return value as an Object to generate an API spec like:
schema:
type: object
diff --git a/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core/src/org/eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/model/ConfigurationSourceType.java b/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core/src/org/eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/model/ConfigurationSourceType.java
index f2305057..b1521dd2 100644
--- a/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core/src/org/eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/model/ConfigurationSourceType.java
+++ b/trace-server/org.eclipse.tracecompass.incubator.trace.server.jersey.rest.core/src/org/eclipse/tracecompass/incubator/internal/trace/server/jersey/rest/core/model/ConfigurationSourceType.java
@@ -60,5 +60,5 @@ public interface ConfigurationSourceType {
+ "front-end needs to provide with corresponding values. "
+ "Use this for complex parameter descriptions instead of parameterDescriptors. "
+ "Omit if not used.")
- String getShema();
+ Object getSchema();
}
We have a similar challenge when creating a configuration instance using a json object (POST).
The core code either needs to support a type of JSON object, or deal with the Map<String, Object> from what it looks like. It would be great to immediately parse the incoming JSON into a POJO based on the configuration source, but I do not see any way to achieve that without pulling in JsonNode
into core.
Saving the parameters as a file or encoding them to a string still forces the configuration source to use a JSON library to parse them, using the same JSON types across core and incubator could be a win.
Is the main limitation with the Map<String, Object>
approach that it can't be validated by the JSON schema?
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.
@awendelin-work I updated the PR to send a json object instead of a json string.
See also #70 for a proposal (to be discussed) on how an JSON object can be provided when posing a configuration.
543470a
to
c3d9d44
Compare
3ddd71f
to
ee213cd
Compare
ee213cd
to
049f390
Compare
...pass/incubator/trace/server/jersey/rest/core/tests/stubs/TmfConfigurationSourceTypeStub.java
Show resolved
Hide resolved
@@ -55,6 +59,7 @@ public class TmfConfigurationSourceTypeStub implements Serializable, ITmfConfigu | |||
public TmfConfigurationSourceTypeStub(@JsonProperty("id") String id, | |||
@JsonProperty("name") String name, | |||
@JsonProperty("description") String description, | |||
@JsonProperty("schema") JsonNode schema, |
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.
Should it be declared after parameterDescriptors?
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.
What difference does it make?
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.
No difference, just to be consistent with the order in TSP (ConfigurationSourceType swagger)...
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.
Done
@@ -66,33 +71,32 @@ public TmfConfigurationSourceTypeStub(@JsonProperty("id") String id, | |||
builder.setConfigParamDescriptors(parameterDescriptors.stream().map(stub -> stub.getConfig()).collect(Collectors.toList())); | |||
} | |||
fConfig = builder.build(); | |||
fSchema = schema; |
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.
Should it be added to the builder and to TmfConfigurationSourceType class?
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.
This would that TmfConfigurationSourceType would require the jackson JSON parser because fSchema
is of type JsonNode
. Besides the TmfConfigurationSourceTypeStub
a client-side implementation for testing the trace server as part of the unit tests.
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.
Ok, if I understand correctly, this stub class is on the client side of the test, not the trace-server side, and it uses TmfConfigurationSourceType as simplification to deserialize the trace-server response?
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.
yes, it's on the client side of the test.
...ncubator/trace/server/jersey/rest/core/tests/stubs/config/TestSchemaConfigurationSource.java
Show resolved
Hide resolved
...ator/internal/trace/server/jersey/rest/core/webapp/TmfConfigurationSourceTypeSerializer.java
Show resolved
Hide resolved
@Schema(description = "A JSON object that describes a schema for parameters that " | ||
+ "the front-end needs to provide with corresponding values. " | ||
+ "Use this for complex parameter descriptions instead of parameterDescriptors. " | ||
+ "Omit if not used.") |
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.
Please mention that the object should adhere to the JSON Schema specification.
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.
Ok
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.
Done
@@ -55,6 +59,7 @@ public class TmfConfigurationSourceTypeStub implements Serializable, ITmfConfigu | |||
public TmfConfigurationSourceTypeStub(@JsonProperty("id") String id, | |||
@JsonProperty("name") String name, | |||
@JsonProperty("description") String description, | |||
@JsonProperty("schema") JsonNode schema, |
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.
No difference, just to be consistent with the order in TSP (ConfigurationSourceType swagger)...
@@ -66,33 +71,32 @@ public TmfConfigurationSourceTypeStub(@JsonProperty("id") String id, | |||
builder.setConfigParamDescriptors(parameterDescriptors.stream().map(stub -> stub.getConfig()).collect(Collectors.toList())); | |||
} | |||
fConfig = builder.build(); | |||
fSchema = schema; |
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.
Ok, if I understand correctly, this stub class is on the client side of the test, not the trace-server side, and it uses TmfConfigurationSourceType as simplification to deserialize the trace-server response?
Signed-off-by: Bernd Hufmann <[email protected]>
See eclipse-cdt-cloud/trace-server-protocol#103 Also, add test for TmfConfigurationSourceType with schema json object. [Added] Serialize TmfConfigurationSourceType schema as json object Signed-off-by: Bernd Hufmann <[email protected]>
049f390
to
36357d3
Compare
2a6eda0
into
eclipse-tracecompass-incubator:master
Support for json-schema in configuration source type:
Requires eclipse-tracecompass/org.eclipse.tracecompass#141
See also
Signed-off-by: Bernd Hufmann [email protected]