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

Make relationship fields in referenced entities nullable #1958

Merged
merged 19 commits into from
Jan 31, 2024

Conversation

Aniruddh25
Copy link
Contributor

@Aniruddh25 Aniruddh25 commented Jan 10, 2024

Why make this change?

  • Closes [Bug]: Cannot return null for non-nullable field. #1747
  • Records in referenced entities may not always be related to the referencing entity. E.g. in a book-websiteplacement relationship, not all books (referenced entity) may have a websiteplacement (referencing) yet. This change is to ensure we get all records of the referenced entity including the ones that DON'T have any relationships.

What is this change?

  • Previously, we used to rely on the nullability of the referenced fields to determine whether the relationship field in the referenced entity should be nullable or not. While this is applicable when we are considering the relationship fields of a referencing entity, it is actually restricting when used for a referenced entity.

  • For example, the referencing entity (BookWebsitePlacement) should have a nullable books (relationship field) based on whether the foreign key book_id in book_website_placements is nullable or not -> indicating whether a website placement MUST have a book or not.

  • On the other hand, the referenced entity books should always have a NULLABLE websiteplacement relationship field in order to include those books that don't have any websiteplacement published yet. Relying on the nullability of the id - the referenced field in the book->book_website_placement foreign key would make the relationship NON-NULLABLE but this restricts inclusion of those books that don't have any website placements hence the error: "Cannot return null for non-nullable field". We need to make the relationship field nullable in such cases.

  • The source entity could be both the referencing and referenced entity
    in case of missing foreign keys in the db or self referencing relationships.

  • Use the nullability of referencing columns to determine the nullability of the relationship field only if

  1. there is exactly one relationship where source is the referencing entity.
    DAB doesn't support multiple relationships at the moment.
    and
  2. when the source is not a referenced entity in any of the relationships.
  • Identifies the scenario where the relationship field is from the referenced entity and always sets its nullability to true.

How was this tested?

Sample Request(s)

{
  books {
    items {
      id
      title
      websiteplacement {
        price
      }
    }
  }
}

BEFORE:
image

AFTER FIX:
There are no more errors and note that we now actually return null as the relationship field value(here, websiteplacement) when querying the referenced entity (here, book) which doesn't have any relationship with the referencing entity.
image

NOTE

The issue is only exposed in a 1:1 relationship. This is because the only other scenario for querying a referenced entity is a 1:many relationship. And for records in the referenced entity(e.g. publisher) which are NOT related to the referencing entity(e.g. book), we explicitly check for nulls and return an empty array. See here:

if (jsonListResult is null)

E.g.

{ publishers {
    items {
      id
      name
      books {
        items {
          title
        }
      }
      }
    }
}

Expected Response
image

NOTE:
When DAB supports multiple relationships, nullability of each relationship field should be determined based on foreign keys associated with each relationship.

@Aniruddh25
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@Aniruddh25
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 6 pipeline(s).

Copy link
Contributor

@severussundar severussundar left a comment

Choose a reason for hiding this comment

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

LGTM

@abhishekkumams
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@Aniruddh25
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@Aniruddh25
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

Copy link
Contributor

@abhishekkumams abhishekkumams left a comment

Choose a reason for hiding this comment

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

LGTM

@Aniruddh25
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@Aniruddh25
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@Aniruddh25
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@seantleonard
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@Aniruddh25
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@Aniruddh25 Aniruddh25 enabled auto-merge (squash) January 31, 2024 01:04
@Aniruddh25 Aniruddh25 merged commit 5fe65f8 into main Jan 31, 2024
14 checks passed
@Aniruddh25 Aniruddh25 deleted the fixNonNullableRelationship branch January 31, 2024 02:24
@julianadormon
Copy link

@Aniruddh25 Hoping you can help :)
I have a website table in my database.
I also have a PaymentProvidersSettings table in my database which is linked to the Website table by WebsiteId.
While a record in PaymentProviderSetting will always be linked to the Website, there may not be any related records (yet).

I updated my dab.config using the following:
dab update Website --relationship "PaymentProviderSettings" --target.entity "PaymentProviderSetting" --cardinality one --relationship.fields "WebsiteId:WebsiteId"

And my schema.graphql shows this as PaymentProviderSettings: PaymentProviderSetting! (non nullable).

When I query the following:

website_by_pk(WebsiteId: $input)
{
TimeZone
CurrencyTypeId
Culture
PaymentProviderSettings {
WebsiteId
PaypalBusinessEmail
}

This returns, "Cannot return null for non-nullable field".
I upgraded to the latest dab, hoping the above may fix the issue, but it has not. I know if it is not quite the same point, but I think my database set-up is pretty standard. Any suggestions would be greatly appreciated.

@julianadormon
Copy link

julianadormon commented Mar 2, 2024

I just ended up making it a list of one, but I'm curious if there is a better way. Thanks!

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.

[Bug]: Cannot return null for non-nullable field.
5 participants