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

Added Required to the Schema for rendering. #3

Merged
merged 4 commits into from
Jan 17, 2024

Conversation

robert-sykes
Copy link
Contributor

The Required option does not seem to be rendering for a property. I am not sure if this is intentional or not. This PR adds the support for it to be rendered out with required being defaulted to true. This allows required properties to be marked as such.

@Ferror
Copy link
Owner

Ferror commented Jan 16, 2024

I'm going to reopen the PR because of CI

@Ferror Ferror closed this Jan 16, 2024
@Ferror Ferror reopened this Jan 16, 2024
@Ferror Ferror force-pushed the add-required-to-generate branch from ac6b84e to 23b664b Compare January 16, 2024 16:49
@Ferror Ferror self-requested a review January 16, 2024 17:55
@Ferror
Copy link
Owner

Ferror commented Jan 16, 2024

There is an issue in asyncapi CLI tool that does not exit code 1 when schema is invalid - asyncapi/cli#1068

Yet, if you look into the pipeline https://github.com/Ferror/asyncapi-doc-bundle/actions/runs/7544841545/job/20539100621?pr=3 you will notice errors. Mostly because the required parameters in schema must not be produced as a property required: true, but as an array of properties. Example:

asyncapi: 2.6.0
info:
  title: 'Service Example API'
  version: 1.2.3
  description: 'This service is in charge of processing user signups'
servers:
  production:
    url: broker.mycompany.com
    protocol: amqp
    description: 'This is production broker.'
  staging:
    url: broker.mycompany.com
    protocol: amqp
    description: 'This is staging broker.'
channels:
  product.created:
    subscribe:
      message:
        $ref: '#/components/messages/ProductCreated'
components:
  messages:
    ProductCreated:
      payload:
        type: object
        properties:
          id:
            type: integer
            description: ''
          amount:
            type: number
            description: ''
          currency:
            type: string
            description: ''
          isPaid:
            type: boolean
            description: ''
          createdAt:
            type: string
            description: ''
            format: date-time
          week:
            type: integer
            description: ''
          payment:
            type: string
            description: ''
          products:
            type: string
            description: ''
          tags:
            type: string
            description: ''
        required:
          - id
          - currency
          - isPaid

@Ferror
Copy link
Owner

Ferror commented Jan 16, 2024

So you are right about the issue, but the solution still needs to be there. Let me know if you would like to continue on this PR or I can provide fix for it 😄

src/Schema.php Show resolved Hide resolved
@robert-sykes
Copy link
Contributor Author

robert-sykes commented Jan 16, 2024

So you are right about the issue, but the solution still needs to be there. Let me know if you would like to continue on this PR or I can provide fix for it 😄

Yep. My mistake. I actually fixed this on my local generator or EventCatalog but completely forgot to come back to fix it here. 😂 happy to apply the fix if you haven't already.

Had myself convinced it was required on the property and not an array of required properties. 🤦

@Ferror
Copy link
Owner

Ferror commented Jan 16, 2024

@robert-sykes go ahead. FYI. I'm starting slowly to build similar library for Java/Spring if you are interested 😄

@robert-sykes robert-sykes requested a review from Ferror January 16, 2024 20:08
@robert-sykes
Copy link
Contributor Author

@robert-sykes go ahead. FYI. I'm starting slowly to build similar library for Java/Spring if you are interested 😄

Ah - i'm afraid I don't know much about Java/Spring to be of any help. 😄

@robert-sykes
Copy link
Contributor Author

robert-sykes commented Jan 16, 2024

@Ferror I think the following line:
return json_encode($this->generator->generate(), JSON_THROW_ON_ERROR | JSON_FORCE_OBJECT);
is causing the items in the array to render as:

  "required": {
        "0": "name",
        "1": "email",
        "2": "age",
        "3": "isCitizen"
    }

and is causing the validation error we are seeing in the actions. I am not sure how get around this at the moment, only once test seems to fail if we remove the JSON_FORCE_OBJECT but I don't know what knock on effects this might have on the rendering.

I've made the change and pushed the fix anyway to check that the Actions are working for now.

@Ferror
Copy link
Owner

Ferror commented Jan 17, 2024

@robert-sykes I will accept the PR and create issue to look into JSON Generator. It is durable, but I think we will need to hack a little bit ;p

@Ferror Ferror merged commit eb8f29a into Ferror:master Jan 17, 2024
6 checks passed
@robert-sykes robert-sykes deleted the add-required-to-generate branch January 17, 2024 16:55
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.

2 participants