You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/02/02 00:12:44 UTC
[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #6485: [WIP] add schema validation mechanism for table config
icefury71 commented on a change in pull request #6485:
URL: https://github.com/apache/incubator-pinot/pull/6485#discussion_r568227370
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/BaseJsonValidator.java
##########
@@ -0,0 +1,56 @@
+package org.apache.pinot.spi.utils.validator;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.github.fge.jackson.JsonLoader;
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import com.github.fge.jsonschema.core.report.ProcessingMessage;
+import com.github.fge.jsonschema.core.report.ProcessingReport;
+import com.github.fge.jsonschema.main.JsonSchema;
+import com.github.fge.jsonschema.main.JsonSchemaFactory;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+abstract public class BaseJsonValidator implements JsonValidator {
+
+ protected JsonSchema _schema;
+ protected ProcessingReport _report;
+
+ protected void loadSchema(String path) {
+ try {
+ JsonNode node = JsonLoader.fromResource(path);
+ JsonSchemaFactory factory = JsonSchemaFactory.byDefault();
+ _schema = factory.getJsonSchema(node);
+ } catch (IOException | ProcessingException e) {
+ e.printStackTrace();
Review comment:
Avoid using printStackTrace, you can use a logger or simply throw the exception to the caller. Same comment for all such instances
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/BaseJsonValidator.java
##########
@@ -0,0 +1,56 @@
+package org.apache.pinot.spi.utils.validator;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.github.fge.jackson.JsonLoader;
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import com.github.fge.jsonschema.core.report.ProcessingMessage;
+import com.github.fge.jsonschema.core.report.ProcessingReport;
+import com.github.fge.jsonschema.main.JsonSchema;
+import com.github.fge.jsonschema.main.JsonSchemaFactory;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+abstract public class BaseJsonValidator implements JsonValidator {
+
+ protected JsonSchema _schema;
+ protected ProcessingReport _report;
+
+ protected void loadSchema(String path) {
+ try {
+ JsonNode node = JsonLoader.fromResource(path);
+ JsonSchemaFactory factory = JsonSchemaFactory.byDefault();
+ _schema = factory.getJsonSchema(node);
+ } catch (IOException | ProcessingException e) {
+ e.printStackTrace();
+ }
+ }
+
+ @Override
+ public boolean validate(String jsonStr) {
+ JsonNode node = null;
+ try {
+ node = JsonUtils.stringToJsonNode(jsonStr);
+ } catch (IOException e) {
+ e.printStackTrace();
Review comment:
Please handle errors correctly - I guess in this case - we should throw ?
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/PinotJsonSchemasValidator.java
##########
@@ -0,0 +1,16 @@
+package org.apache.pinot.spi.utils.validator;
+
+public class PinotJsonSchemasValidator extends BaseJsonValidator {
+
+ private static final String BASE_SCHEMA_DIR = "/schemas";
+
+ private static final String TABLE_CONFIG_SCHEMA = BASE_SCHEMA_DIR + "/tableConfig.json";
Review comment:
Do we need 2 separate constants for this ?
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/BaseJsonValidator.java
##########
@@ -0,0 +1,56 @@
+package org.apache.pinot.spi.utils.validator;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.github.fge.jackson.JsonLoader;
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
+import com.github.fge.jsonschema.core.report.ProcessingMessage;
+import com.github.fge.jsonschema.core.report.ProcessingReport;
+import com.github.fge.jsonschema.main.JsonSchema;
+import com.github.fge.jsonschema.main.JsonSchemaFactory;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+abstract public class BaseJsonValidator implements JsonValidator {
Review comment:
nit: please add javadoc
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/JsonValidator.java
##########
@@ -0,0 +1,13 @@
+package org.apache.pinot.spi.utils.validator;
+
+import java.io.Serializable;
+import java.util.List;
+
+
+public interface JsonValidator extends Serializable {
Review comment:
Needs javadoc
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/validator/JsonValidator.java
##########
@@ -0,0 +1,13 @@
+package org.apache.pinot.spi.utils.validator;
+
+import java.io.Serializable;
+import java.util.List;
+
+
+public interface JsonValidator extends Serializable {
Review comment:
Maybe use a better name for this interface ? Do we expect this to be a common interface for any kind of json ? Or just limited to config (table or schema) ?
If its just config - we could also say PinotConfigValidator or something ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org