Skip to content
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

Add ?pretty URL extension. Add /schemas/ids/:id/schema #876

Closed
wants to merge 1 commit into from
Closed

Add ?pretty URL extension. Add /schemas/ids/:id/schema #876

wants to merge 1 commit into from

Conversation

OneCricketeer
Copy link
Contributor

@OneCricketeer OneCricketeer commented Aug 26, 2018

Enhancements over

Utilize the SchemaRegistryResourceExtension class to inject Jackson's ObjectMapper & PrettyPrinter.

No longer do we have to use CLI pipes into jq / python -m json.tool to format outputs from the CLI!!

Example

Not only do GET's work, but also POSTs. I've went over the using document to update the entire page.

Without ?pretty

$ curl -X GET http://localhost:8081/subjects/LogLine-value/versions/1/schema
{"type":"record","name":"LogLine","namespace":"JavaSessionize.avro","fields":[{"name":"ip","type":"string"},{"name":"timestamp","type":"long"},{"name":"url","type":"string"},{"name":"referrer","type":"string"},{"name":"useragent","type":"string"},{"name":"sessionid","type":["null","int"],"default":null}]}

And with

$ curl -X GET http://localhost:8081/subjects/LogLine-value/versions/1/schema?pretty
{
  "type" : "record",
  "name" : "LogLine",
  "namespace" : "JavaSessionize.avro",
  "fields" : [ {
    "name" : "ip",
    "type" : "string"
  }, {
    "name" : "timestamp",
    "type" : "long"
  }, {
    "name" : "url",
    "type" : "string"
  }, {
    "name" : "referrer",
    "type" : "string"
  }, {
    "name" : "useragent",
    "type" : "string"
  }, {
    "name" : "sessionid",
    "type" : [ "null", "int" ],
    "default" : null
  } ]
}

Details

  1. Added new URL for /schemas/ids/:id/schema, which was missing from issue#629: endpoint to return schema literal #686.

  2. Updated the RestClient methods that returned String type because the other object types are handled by the PrettyQueryExtension class.

  3. Added a UrlParser that for now is used to parse out query parameters.

  4. Updated documentation ==> Closes Document the schema's JSON in a pretty way #847

@ghost
Copy link

ghost commented Aug 26, 2018

@confluentinc It looks like @Cricket007 just signed our Contributor License Agreement. 👍

Always at your service,

clabot

Checking if a Schema Is Registered Under Subject "Kafka-key"
------------------------------------------------------------
--------------------------------------------------------------
Checking if a Schema Is Registered Under Subject "Kafka-value"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key still had version 1 and all the earlier examples update the value, so I think this was a typo.

@@ -439,7 +477,7 @@ The compatibility resource allows the user to test schemas for compatibility aga

.. http:post:: /compatibility/subjects/(string: subject)/versions/(versionId: version)

Test input schema against a particular version of a subject's schema for compatibility. Note that the compatibility level applied for the check is the configured compatibility level for the subject (``http:get:: /config/(string: subject)``). If this subject's compatibility level was never changed, then the global compatibility level applies (``http:get:: /config``).
Test input schema against a particular version of a subject's schema for compatibility. Note that the compatibility level applied for the check is the configured compatibility level for the subject (``GET /config/(string: subject)``). If this subject's compatibility level was never changed, then the global compatibility level applies (``GET /config``).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor confusing doc fixing.


.. http:post:: /subjects/(string: subject)/versions

Register a new schema under the specified subject. If successfully registered, this returns the unique identifier of this schema in the registry. The returned identifier should be used to retrieve this schema from the schemas resource and is different from the schema's version which is associated with the subject.
If the same schema is registered under a different subject, the same identifier will be returned. However, the version of the schema may be different under different subjects.

A schema should be compatible with the previously registered schema or schemas (if there are any) as per the configured compatibility level. The configured compatibility level can be obtained by issuing a ``GET http:get:: /config/(string: subject)``. If that returns null, then ``GET http:get:: /config``
A schema should be compatible with the previously registered schema or schemas (if there are any) as per the configured compatibility level. The configured compatibility level can be obtained by issuing a ``GET /config/(string: subject)``. If that returns null, then ``GET /config``
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor confusing doc fixing.

.map(s -> Arrays.copyOf(s.split("="), 2))
.collect(groupingBy(s -> s[0],
mapping(s -> s[1] == null ? "" : s[1], toList())));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Building over Kafka 2.0+, so Stream API support 👍

// no-op
}
return null;
}).collect(toList());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Building over Kafka 2.0+, so Stream API support 👍

@OneCricketeer
Copy link
Contributor Author

OneCricketeer commented Aug 29, 2018

Looking for feedback

Would adding the extension to the other resource list be cleaner?

-    schemaRegistryResourceExtensions =
-        schemaRegistryConfig.getConfiguredInstances(
-            schemaRegistryConfig.definedResourceExtensionConfigName(),
-            SchemaRegistryResourceExtension.class);
-
     config.register(RootResource.class);
     config.register(new ConfigResource(schemaRegistry));
     config.register(new SubjectsResource(schemaRegistry));
@@ -82,16 +76,17 @@
     config.register(new SubjectVersionsResource(schemaRegistry));
     config.register(new CompatibilityResource(schemaRegistry));
 
-    try {
-      new PrettyQueryExtension()
-              .register(config, schemaRegistryConfig, schemaRegistry);
-    } catch (SchemaRegistryException e) {
-      log.error(String.format(
-              "Error registering ?%s query resource extension. Starting up without it",
-              QUERY_PARAM_PRETTY), e);
+    schemaRegistryResourceExtensions = new ArrayList<>();
+    schemaRegistryResourceExtensions.add(new PrettyQueryExtension());
+
+    List<SchemaRegistryResourceExtension> extensions =
+            schemaRegistryConfig.getConfiguredInstances(
+                    schemaRegistryConfig.definedResourceExtensionConfigName(),
+                    SchemaRegistryResourceExtension.class);
+    if (extensions != null) {
+      schemaRegistryResourceExtensions.addAll(extensions);
     }
 
-    if (schemaRegistryResourceExtensions != null) {
     try {
       for (SchemaRegistryResourceExtension
           schemaRegistryResourceExtension : schemaRegistryResourceExtensions) {
@@ -101,7 +96,7 @@
       log.error("Error starting the schema registry", e);
       System.exit(1);
     }
-    }

@OneCricketeer OneCricketeer changed the title Add ?pretty URL extension Add ?pretty URL extension. Add /schemas/ids/:id/schema Sep 11, 2018
@OneCricketeer
Copy link
Contributor Author

@mageshn Could I get a review once I get the tests passing again?

@mageshn mageshn requested review from mageshn and a team October 15, 2018 22:59
@@ -47,7 +49,15 @@ public void setSchemaString(String schemaString) {
this.schemaString = schemaString;
}

public String toJson() throws IOException {
public String toJson(boolean pretty) throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I need this

Suggested change
public String toJson(boolean pretty) throws IOException {
public String toJson() throws IOException {

@mageshn mageshn requested review from rayokota and removed request for mageshn November 8, 2018 21:51
Copy link
Member

@rayokota rayokota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Cricket007 , we're in the process of making Schema Registry pluggable, so that schemas are not necessarily JSON format (such as Protobuf). Since these changes assume JSON schemas, we're going to hold off on merging this PR. We can return to whether a pretty query param makes sense after SR is pluggable. Until then you can try using a custom REST extension to get the behavior that you want. Thanks!

@rayokota
Copy link
Member

rayokota commented Nov 9, 2018

I'm closing this PR for now but feel free to reopen if you have further changes or suggestions. Thanks!

@rayokota rayokota closed this Nov 9, 2018
@OneCricketeer
Copy link
Contributor Author

@rayokota It might be possible for me to extend to detect the response header to see if it's actually JSON that needs formatted

@rayokota
Copy link
Member

rayokota commented Nov 9, 2018

@Cricket007 , you can see the initial PR for making SR pluggable here: #927. Probably once this gets merged, we can consider adding a toString(boolean pretty) to ParsedSchema, if we want to continue to explore prettifying the result in a general fashion.

@OneCricketeer
Copy link
Contributor Author

#927. Probably once this gets merged, we can consider adding a toString(boolean pretty) to ParsedSchema

@rayokota - Worth reopening / rebasing this ?

@rayokota
Copy link
Member

Hi @Cricket007 , there is now a ?format={format} query param for /ids/{id}. The format gets passed to ParsedSchema.formattedString(String format).

If you still want a /ids/{id}/schema?format={format} endpoint, PRs are welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document the schema's JSON in a pretty way
2 participants