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

Improve POST /Groups Validation Feedback #252

Open
tsteinholz opened this issue Sep 6, 2022 · 2 comments
Open

Improve POST /Groups Validation Feedback #252

tsteinholz opened this issue Sep 6, 2022 · 2 comments

Comments

@tsteinholz
Copy link

Tested Versions:

  • pravega/schemaregistry:0.2.0
  • pravega/schemaregistry:0.4.0

Problem description

POST: http://{schema registry uri}:9092/v1/groups

{
   "groupName": "mygroup",
   "groupProperties": {
      "serializationFormat":{
         "serializationFormat":"Json"
      },
      "compatibility":{
         "policy":"BackwardTransitive"
      },
      "allowMultipleTypes":false,
      "properties":{ }
   }
}

HTTP 400 Error when creating a group using the "Json" serialization format. Server Output:
[grizzly-http-server-0] WARN i.p.s.s.r.r.AbstractResource - Bad argument for request createGroup failed with exception: . null. {}

HTTP 500 Error when creating a group with a "JSON" or "json" serialization format.

2022-09-06 17:46:22,042 278827 [RestServer RUNNING] WARN  i.p.s.s.r.r.AbstractResource - Request createGroup failed with exception:  failed with Internal Server error.
java.lang.NullPointerException: null
	at io.pravega.schemaregistry.contract.transform.ModelHelper.decode(ModelHelper.java:65)
	at io.pravega.schemaregistry.contract.transform.ModelHelper.decode(ModelHelper.java:251)
	at io.pravega.schemaregistry.server.rest.resources.GroupResourceImpl.lambda$createGroup$10(GroupResourceImpl.java:138)
	at io.pravega.schemaregistry.server.rest.resources.AbstractResource.lambda$withAuthorization$1(AbstractResource.java:77)
	at java.base/java.util.concurrent.CompletableFuture$UniCompose.tryFire(Unknown Source)
	at java.base/java.util.concurrent.CompletableFuture.postComplete(Unknown Source)
	at java.base/java.util.concurrent.CompletableFuture$AsyncRun.run(Unknown Source)

Problem location

API Rest Controller for Groups (POST), possibility the i.p.s.s.r.r.AbstractResource.

Suggestions for an improvement

The user gets no feedback in the HTTP return as to what happened, only the return code. There is an unhandled exception when using invalid serialization formats and it is not clear to me if the schema registry is able to create schemas with the JSON serialization format or if there is another validation error preventing the group from being defined.

The current payload is based of the (invalid) example provided here https://pravega.io/docs/snapshot/schema-registry/rest-usage

@tsteinholz
Copy link
Author

tsteinholz commented Sep 8, 2022

I found the error was coming from the following line of code throwing an IllegalArgumentException with no message in it. Resulting in the "null. {}" message being returned to the user.

        Preconditions.checkArgument(isValidCompatibilityForFormat(groupProperties.getSerializationFormat(), groupProperties.getCompatibility()));

src: https://github.com/pravega/schema-registry/blob/master/server/src/main/java/io/pravega/schemaregistry/service/SchemaRegistryService.java#L143

The source of this issue seems to be the inconsistent usage of the isValidCompatibilityForFormat method.

    private boolean isValidCompatibilityForFormat(SerializationFormat serializationFormat, Compatibility compatibility) {
        switch (serializationFormat) {
            case Avro:
                return true;
            case Protobuf:
                // Only Allow Any or Deny All are allowed values. 
            case Json:
                // Only Allow Any or Deny All are allowed values. 
            case Custom:
                // Only Allow Any or Deny All are allowed values. 
            case Any:
                return compatibility.getType().equals(Compatibility.Type.AllowAny) || compatibility.getType().equals(Compatibility.Type.DenyAll);
            default:
                throw new IllegalArgumentException("Unknown serialization format");
        }
    }

src:

private boolean isValidCompatibilityForFormat(SerializationFormat serializationFormat, Compatibility compatibility) {

It is documented to return a boolean if the schema is valid, however, it also throws an IllegalArgumentException with a description.

Since the method is being called inside of a Preconditions.checkArgument the IllegalArgumentException with a description ("Unknown serialization format") is actually resulting in an HTTP 500 error to the end user: IllegalArgumentException(null) instead of the useful description.

In the case that the isValidCompatibilityForFormat does return that it is not valid, it does not tell the user why - Preconditions.checkArgument returns a null IllegalArgumentException, resulting in no description being sent to the user, such as "Compatibility Type must be 'AllowAll' or 'DenyAll' for " in the HTTP 400 error.

Once I was able to figure out why I was getting an HTTP 400 error, updating the compatibility type to "AllowAll" fixed my issue in this case, where the following provided me a HTTP 201 return.

{
    "groupName": "mygroup",
    "groupProperties": {
        "serializationFormat": {
            "serializationFormat": "Json"
        },
        "compatibility": {
            "policy": "AllowAny"
        },
        "allowMultipleTypes": true,
        "properties": {}
    }
}

This would be extremely helpful to have documented in the

I will leave this issue open to associate an improvement PR to these suggestions.

@tsteinholz tsteinholz changed the title Cannot Create a Group with Json serialization Improve POST /Groups Validation Feedback Sep 8, 2022
@shshashwat
Copy link
Contributor

@tsteinholz, thank you for the input. We will have a look on this and will work accordingly.

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

No branches or pull requests

2 participants