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

Please create objects in the definition section to be used #1123

Open
AceHack opened this issue Apr 8, 2020 · 11 comments · May be fixed by #1523
Open

Please create objects in the definition section to be used #1123

AceHack opened this issue Apr 8, 2020 · 11 comments · May be fixed by #1523
Assignees

Comments

@AceHack
Copy link

AceHack commented Apr 8, 2020

I would have thought the following would work:

{
  "$schema": "http://json-schema.org/draft-07/schema",
  "type": "object",
  "additionalProperties": false,
  "definitions": {
    "MessageAttribute": {
      "type": "object",
      "additionalProperties": true,
      "properties": {
        "Type": {
          "type": "string"
        },
        "Value": {
          "type": "string"
        }
      }
    }
  },
  "properties": {
    "MessageAttributes": {
      "existingJavaType": "java.util.Map<String, MessageAttribute>",
      "type": "object",
      "additionalProperties": { "$ref": "#/definitions/MessageAttribute" }
    }
  }
}

But the MessageAttribute class is not created so the existingJavaType fails. I can work around this problem today in an ugly way but it would be nice for the above syntax to work. The ugly way is to move everything in the definitions section to another file as below:

message-attribute.json

{
  "$schema": "http://json-schema.org/draft-07/schema",
  "type": "object",
  "additionalProperties": true,
  "properties": {
    "Type": {
      "type": "string"
    },
    "Value": {
      "type": "string"
    }
  }
}

message.json

{
  "$schema": "http://json-schema.org/draft-07/schema",
  "type": "object",
  "additionalProperties": false,
  "properties": {
    "MessageAttributes": {
      "existingJavaType": "java.util.Map<String, MessageAttribute>",
      "type": "object",
      "additionalProperties": { "$ref": "message-attribute.yaml" }
    }
  }
}
@eirnym
Copy link

eirnym commented Apr 9, 2020

it's not really because of MessageAttribute has not been created. This is because existingJavaType can't sometimes resolve MessageAttribute.

The additional problem is classes are moved around more likely chaotically rather than under your guidance, see #1107 for this.

@dewthefifth
Copy link
Collaborator

I suspect you're right and the "definitions" section is not being processed. However, I haven't dug into the code to confirm that. I will say, what you call a messy work around is actually my preferred organization. I've always defined one class, one struct, one schema per file and it just feels more natural to me than defining an entire data model in a single file (although I concede XML kind of endorsed that kind of behavior).

That said, there are a few things I noticed about your schema:

  • As Multiple subpackages #1107 points out, it can be difficult to guess which package a type will be created in when that type doesn't have have an explicit package declared.
  • Your use of "existingJavaType": "java.util.Map<String, MessageAttribute>", does not fully qualify the MessageAttribute type, which could result in a missing import if the MessageAttribute is not in the same package as your Message type, which appears to be named implicitly by the file name as opposed to being explicitly declared.
  • My understand of existingJavaType is that it's for use when the declared type already exist on the classpath at the time generation is occurring. In your case, the class you're trying to use has not yet been defined, particularly if we were to process definitions after their enclosing class (for example if they were being created as InnerClasses).

@joelittlejohn
Copy link
Owner

When jsonschema2pojo was created, 'definitions' was not part of the standard. Many people started using 'definitions' to hold extra schemas but we didn't add support here because it was just a common pattern but not part of the standard.

Now it has been brought into the standard I don't have a problem with adding support for it. Over the years a lot of people have expected it to work (and it appears that many people have a schema document with only definitions, and no root schema to reference them).

I'd probably just bump to 1.1.x for this change.

@dewthefifth dewthefifth self-assigned this Apr 10, 2020
@dewthefifth
Copy link
Collaborator

I'll take a look at it this weekend and try to get an implementation. I haven't tinkered in those parts of the code before, so I'm not sure what the LOE will be, but don't mind getting my hands dirty.

That said, where do we envision the generated classes being generated?

  • As inner classes of the defining schema
  • As their own classes in the same package as the defining schema
  • In a package named after the defining schema, but in their own classes

I feel like inner class is the way to go because you could have multiple schema with definitions using the same name, defined in the same package. I'm not sure how the JSON schema treats that behavior, or whether it's allowed. I'm not sure the JSON schema even discusses how schemas should relate to one another when not referencing one another.

If I generate them as inner classes, which makes the most sense to me, or in their own classes within the same package what should their visibility be?

  • private
  • protected
  • public
  • package default

I guess I'm putting forward the following suggesting and looking for input:

{
  "$schema": "http://json-schema.org/draft-07/schema",
  "type": "object",
  "additionalProperties": false,
  "definitions": {
    "MessageAttribute": {
      "type": "object",
      "additionalProperties": true,
      "properties": {
        "Type": {
          "type": "string"
        },
        "Value": {
          "type": "string"
        }
      }
    }
  },
  "properties": {
    "MessageAttributes": {
      "existingJavaType": "java.util.Map<String, MessageAttribute>",
      "type": "object",
      "additionalProperties": { "$ref": "#/definitions/MessageAttribute" }
    }
  }
}

becomes

public class Message
{
  private static class MessageAttribute
  {
    private String type;
    private String value;
    private Map<String, Object> additionalProperties;
  }

  private Map<String, MessageAttribute> messageAttributes;
}

@joelittlejohn
Copy link
Owner

I don't think we should use inner classes, this pattern isn't used anywhere else. It also creates a more serious breaking change. I think these should go into whatever package is currently in context when we're processing the enclosing schema.

@dewthefifth
Copy link
Collaborator

I'll implement them as classes inside the current context, and make the assumption that they will not conflict with the "definitions" of another schema in that same context.

IE I'm going to expect it to throw an exception if two schemas in the same context have competing definitions.

@dewthefifth
Copy link
Collaborator

dewthefifth commented Apr 12, 2020

@joelittlejohn I took a stab at implementing this on #1124 however the ExtendsIT unit test fails after my implementation because it looks like there's already a sub-implementation for definitions when the definition is referenced directly by an extends call from within the actual schema.

Specifically, ExtendsIT has the following schema which processes with the current unit test.

{
  "definitions": {
    "parent": {
      "type": "object",
      "properties": {
        "propertyOfParent": {
          "type": "string"
        }
      }
    },
    "child": {
      "type": "object",
      "extends": {"$ref": "#/definitions/parent"},
      "properties": {
        "propertyOfChild": {
          "type": "string"
        }
      }
    }
  },
  "type": "object",
  "properties": {
    "child": {"$ref": "#/definitions/child"}
  }
}

My first stab at an implementation used the Schema.deriveChildSchema to define the schema used for definitions classes, which worked for my implemented unit tests but broke the existing ExtendsIT class because the ExtendsIT unit test ends up generating an invalid schema path when it encounters {"$ref": "#/definitions/parent"}. I dug deeper and determined that the current implementation makes use of the #definitions/parent portion of the schema to generate sub-schemas from within a larger file. I adjusted my schema definition to use the same convention, and stopped getting the path does not exist exception. However, I am not getting an assertion error out of the ExtendsIT unit test, because it looks like Parent is being processed twice. The first processing appears to come from my new code, where the schema is identified as #/definitions/parent and the second processing appears to be #/definitions/child/extends when the child class is processed (again by my new code). The end result is that I end up with a Parent.java and a Parent__1.java.

I'm hoping you can provide some insight as to the specifics of why the child identifies it's extension as #/definitions/child/extends instead of as #/definitions/parent, and provide some guidance as to how you'd like the issue resolved.

For reference, the following PR is where my existing changes live: #1124

@rfelgent
Copy link

rfelgent commented Oct 9, 2020

Hello everyody,

is there a chance to get this done for 1.1.0 ?

@yelhouti
Copy link

yelhouti commented May 31, 2022

Any news on this, I have a file that looks like:

{
  "allOf": [
    {
      "$ref": "#/definitions/Check"
    }
  ],
  "definitions": {
    "Check": {
"description": "The model for a Check",
      "type": "object",
      "required": [
        "header"
      ],
      "properties": {

     }
      

EDIT:
I managed to make it work by moving the referenced definition (Check) as the top level object. But it seems like this is quite standard and should work.

@dkellenb
Copy link

dkellenb commented Nov 2, 2022

Is there any plan to implement the feature to force generation of unreferenced parts declared in the #definitions using a configuration?

@lejtemxviw
Copy link

This would be useful for us, as it would allow us to generate Java classes from the definitions in a swagger/OAS2 specification (which I don't believe is possible otherwise, without moving all those definitions into separate files...)

unkish added a commit to unkish/jsonschema2pojo that referenced this issue Aug 2, 2023
unkish added a commit to unkish/jsonschema2pojo that referenced this issue Nov 4, 2023
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 a pull request may close this issue.

8 participants