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 uniqueness validation for column #87

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

syou6162
Copy link
Contributor

When a developer is entering column information into yaml, sometimes duplicate column names are entered. For example, this is the case.

version: 2
models:
  - name: my_model
    columns:
      - name: user_id
      - name: user_name
      - ...
      - name: user_id

I wanted dbt-jsonschema to check for such cases, so I added "uniqueItems": true.

@joellabes
Copy link
Collaborator

Good idea @syou6162! I have just read up on uniqueItems, and it looks like it checks the entire object for equality, not just the key: https://stackoverflow.com/a/57677429. This means that as soon as you start adding descriptions/tests/etc to the columns, the second column won't mark as duplicate anymore.

It looks like there is a setting which behaves the way you want, discussed here: json-schema-org/json-schema-vocabularies#22

However that doesn't seem to be implemented by many/any validators which isn't very helpful. Because of this I think we should leave this unmerged - wdyt?

@syou6162
Copy link
Contributor Author

This means that as soon as you start adding descriptions/tests/etc to the columns, the second column won't mark as duplicate anymore.

@joellabes What you say is correct.

However, I would be happy if this pr could be merged, because checking uniqueItems is better than not checking it, because it can detect invalid yaml files in cases like the one I wrote.

Of course, if the policy of this repository is to not include half-baked implementations, I will close this pr without merging it (I don't care!).

@joellabes
Copy link
Collaborator

Yeah I can get on board with it as "better than nothing" - let's do it!

@joellabes joellabes merged commit fa8f55c into dbt-labs:main Aug 28, 2023
11 checks passed
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