Composite key executes an invalid query if one key is null

[DiscountDarcy]

We have an entity called qEventSignup that represents either a player (child) or a user (adult) signing up for some event.

Event signups are attached to 'question answers' -- items filled out on the signup form. The foreign key is not the event signup ID but rather a composite key of the event ID and either the child or userID signing up, such that if we delete the signup for whatever reason, if they sign up again we have their old data. So we have these relationships:

	function questionAnswersUser() {
		return hasMany(
			relationName = "qEventAnswer",
			foreignKey = [ 'eventID', 'userID' ],
			localKey = [ 'eventID', 'userID' ]
		);
	}

	function questionAnswersChild() {
		return hasMany(
			relationName = "qEventAnswer",
			foreignKey = [ 'eventID', 'childID' ],
			localKey = [ 'eventID', 'childID' ]
		);
	}

If I load an event signup for a child and try to call getQuestionAnswersUser(), it will try to execute the query even though one of the foreign keys is null:

	var test = wirebox
			.getInstance( "qEventSignup" )
			.findOrFail( '5901b7de-bcfb-4dec-9d3d-c96701a34c01' );
			
			dump( var = test.getQuestionAnswersUser(), top = 1 );

Throws Conversion failed when converting from a character string to uniqueidentifier with the following SQL:

SELECT [event_answers].[answer], [event_answers].[childID], [event_answers].[clientID], [event_answers].[eventID], [event_answers].[id], [event_answers].[linkID], [event_answers].[questionID], [event_answers].[userID] FROM [event_answers] WHERE ([event_answers].[eventID] = 'BF26433E-F36B-1410-8386-00E7D4F45408' AND [event_answers].[eventID] IS NOT NULL AND [event_answers].[userID] = '' AND [event_answers].[userID] IS NOT NULL)

If, however, I eager load both relationships:

	var test = wirebox
			.getInstance( "qEventSignup" )
			.findOrFail( '5901b7de-bcfb-4dec-9d3d-c96701a34c01' )
                  	.with( [ "questionAnswersUser.question", "questionAnswersChild.question" ] )
			
			dump( var = test.getQuestionAnswersUser(), top = 1 );	

Then it will correctly give me an empty array.

I know Quick has added a lot with respect to not even executing queries when a key is null (and I vaguely remember working on a PR that touched on this) but it seems like a tricky question as to how Quick should behave when you specify a composite key and one of them is null. I could see cases where the relationship might still be 'good', even though I wouldn't architect a table that way; but in my case (and, I hope, most cases?) if you tell a relationship that it needs two keys and you haven't got two keys, there's nothing there and we don't even need to check.

Perhaps an option on the relationship entity like compositeKeysMustNotBeNull?

[elpete]

I agree with you that this is tricky since if at least one key in a composite key has a value, it might be legitimate.

I think perhaps the issue is that we don't have a good way to use a null value with a uniqueidentifier column. If that was, for instance, a string column it would correctly pass null to one of them and the relationship would either get back data or not.

It's also interesting that we add the AND {column} IS NOT NULL to the query. That makes sense for single key relationships. Not as much for composite keys (as long as one key is not null).

I'm not sure how much work this would be, but perhaps we can convert the query so that instead of

WHERE (
    [event_answers].[eventID] = { value: 'BF26433E-F36B-1410-8386-00E7D4F45408', cfsqltype: "uniqueidentifier" }
    AND [event_answers].[eventID] IS NOT NULL
    AND [event_answers].[userID] = { value: '', cfsqltype: 'uniqueidentifier', null: true }
    AND [event_answers].[userID] IS NOT NULL
)

(which even plugging in the query params show how this makes no sense) we could convert the null query param to an IS NULL check

WHERE (
    [event_answers].[eventID] = { value: 'BF26433E-F36B-1410-8386-00E7D4F45408', cfsqltype: "uniqueidentifier" }
    AND [event_answers].[eventID] IS NOT NULL
    AND [event_answers].[userID] IS NULL
)

I think that is a way to handle these.

In summary:

1. Check if single key: if so, current flow where we don't even try to execute a query if the value is null.
2. Check if at least one of the keys in the composite key is not null. If they are all null, don't even try to execute a query.
3. Convert all null keys in the composite keys to whereNull checks.
4. Execute the query.

I think this solves this potential use case. Now, what it doesn't help with is that it will try to execute a query in your particular use case because a single non-null key is all that is needed in a composite key. I'm not sure the best way to handle that scenario. The extra parameter is a possibility, but I'd like to avoid it if possible. How bad would it be if it executed the query if one value was null? Could your relationship perhaps check for this and return a different relationship value?

[DiscountDarcy]

This is not a common scenario for us, but the way that makes sense to me to handle it is to flag the relationship as either requiring both keys, or not; while it is conceivable that a multi-key relationship could be legit if only one key has data, I'd rather define that in the relationship just as I'm defining that it's a multi-key relationship. Not only for Quick to handle it correctly, but for the same reason I don't let Quick determine foreign & local keys -- I want another developer to take one look at the relationship and know what there is to know about it.

It's not the end of the world if it executes a query if one value is null. That's still better than what happens now. But in a multi-key relationship, it seems like a valuable piece of metadata is whether the relationship says "enforce the requirement of both keys" or "here are two keys but one is good enough" -- which, FWIW, I don't think we ever have or that I've ever actually seen.

[elpete]

I don't know exactly how it would look, but it would be ideal in this scenario to be able to mark a key as optional. Here's some psuedo-code:

function questionAnswersUser() {
    return hasMany(
	relationName = "qEventAnswer",
	foreignKey = [ 'eventID', optionalKey('userID') ],
	localKey = [ 'eventID', optionalKey('userID') ]
    );
}

This way the relationship would know which keys may be null and still valid. I like the idea of handling this when declaring the keys since we can specify which keys are required and which are not. I don't know the best way to go about this. The optionalKey function there could be a function on the BaseEntity and we could change the code to handle them. Again, I like this direction, but I'm still wondering if this is the right call.

Additionally, what would it say if one of the foreignKeys was optional but not the corresponding localKey? Is that allowed? Scenarios like that would need to be worked through.

What are your thoughts on that?

[DiscountDarcy]

I can't think of a better idea than optionalKey().

On the foreign key / local key question, I'd say that's good to keep in mind but let's not let the perfect be the enemy of the good. Solving just the optionalKey piece will make that problem more approachable and easier to visualize / test for.

[DiscountDarcy]

This is really two issues -- what to do in the case of a composite key, and what to do if you have a single foreign uniqueidentifier key that can be NULL.

From Slack:

We have a User object that stores a 3rd party vendor key, which previously was a string. Then we have 3rd party data related to User on another table, with that string as the foreign key. Not all users will have this 3rd party key. This works fine, e.g. where the 3rd party key is AYSOID:

function volunteerCertifications() {
	return hasMany( relationName = "qVolunteerCertification", localKey = "AYSOID", foreignKey = "AYSOID" );
}

On users who do not have AYSO IDs, this returns SQL like:

SELECT [bunchOfFields] from [volunteer_certifications] WHERE ([volunteer_certifications].[AYSOID] = '' AND [volunteer_certifications].[AYSOID] IS NOT NULL)

The '' bit isn't super necessary since it's canceled out by the IS NOT NULL anyway but I see how it gets added.

The 3rd party changed their keys to be GUIDs. Now the same query fails, because it's still trying to add WHERE AYSOID = ''

Which in MS SQL will throw Conversion failed when converting from a character string to uniqueidentifier.

Two workarounds:

1. If you're working with a single object, just put in a length/guid check in the relationship function and return the empty array so you don't even hit the database, since you already know you can't have any data related by a key that is null;

2. Eager-load volunteerCertifications using .with() since this causes QB to query the relationship in a different manner that correctly handles null values on the parent object.

In thinking about the "correct" move here, it seems like it probably just shouldn't even generate a query at all if they key is not composite; it should see that the key is NULL and return an empty array.