forcedotcom / quiz-host-app

Multiplayer quiz app built on Salesforce technology (host app)
Creative Commons Zero v1.0 Universal
108 stars 67 forks source link

Can create multiple answers for same player same question #8

Closed annyhe closed 5 years ago

annyhe commented 5 years ago
Screen Shot 2019-09-23 at 11 35 36 AM

Insert succeeds if the answers are created in rapid succession

Ways to solve it:

pozil commented 5 years ago

@annyhe how did you test this? The answers should be created by calling the AnswerRestResource REST API. This means that each call is isolated in its own transaction so there can't be duplicates.

annyhe commented 5 years ago

@pozil I was playing with two players, one in incognito chrome and another in Firefox in private window mode. Clicked the answer in Firefox in rapid succession and the duplicate answer was able to go through. Will try to replicate it again.

pozil commented 5 years ago

ok, maybe my duplicate answer checking code is wrong?

annyhe commented 5 years ago

The DuplicateException is working when the user submits their answer more than 2 seconds apart. It doesn't seem to guard against double clicks sending duplicate answers. What can help:

  1. A solution on the LWC OSS side is to debounce the requests. So the user cannot make two requests within 1 second of each other, for example.
  2. Another solution is to go through on the Salesforce side, during the score evaluation, and discard the duplicate answers. Say we only keep the earliest answer.

Which one do you prefer @pozil ?

pozil commented 5 years ago

I'll do option #1 whatever happens. I just haven't implemented the question/answer UI transition yet.

Can you go for a third option and have that duplicate answer check in a trigger? I'd like to keep the data clean and I also want to make the app robust since it will be open-sourced (#NoCheaters).

pozil commented 5 years ago

And thanks for catching that, I thought that we were safe.

annyhe commented 5 years ago

Hello @pozil I've looked into the triggers a bit more. The trigger is run per each REST API request, so double clicking on an answer results in two separate API calls, with each call processed by the trigger separately. HenceanswerService.getFromPlayer(answer.Player__c, answer.Question__c); in Trigger yields no record found, even when there's a request with duplicate information, which will be created at the same time .

Screen Shot 2019-09-25 at 9 42 27 AM

I'll look into it more and propose alternative strategies.

annyhe commented 5 years ago

@pozil While we can find the duplicates in after-insert trigger on Quiz_Answer__c, the solution is not scalable as each API request causes the Trigger to run, resulting in System.LimitException: Too many SOQL queries

            try {
                // finds one record or throws error
                answerService.getFromPlayer(answer.Player__c, answer.Question__c);
            } catch (AbstractDataService.TooManyRecordFoundException e) {
                // DML on Trigger records is not allowed, hence 'create' record
                Quiz_Answer__c recordToDelete = new Quiz_Answer__c(
                    Id = answer.Id,
                    Player__c = answer.Player__c, 
                    Question__c = answer.Question__c, 
                    Answer__c = answer.Answer__c);
                deleteThese.add(recordToDelete);
            }          

Deleting duplicate answers when Quiz Session Phase == PostQuestion works. Please review PR https://github.com/pozil/quiz-sfdx/pull/11

pozil commented 5 years ago

Hold on, why do it after insert and not before insert? I don't understand how you get too many SOQL queries. Is your trigger code bulkified?

Here's what I'm thinking in terms of pseudo code:

trigger QuizAnswerTrigger on Quiz_Answer__c (before insert) {
  Set<Id> playerIds = getUniquePlayerIds(Trigger.new);
  List<Quiz_Answer__c> existingAnswers = getAnswersFromPlayerIds(playerIds);
  for (Quiz_Answer__c answer : Trigger.new) {
    if (existingAnswers.contains(answer)) {
      answer.addError('Duplicate answer');
    }
  }
}

This logic only requires 2 SOQL queries so you shouldn't bump into limits. Did I miss something?

annyhe commented 5 years ago

@pozil The issue with before insert on the QuizAnswerTrigger is it runs every time a single Quiz Answer record is created. So in the case of multiple REST API coming in, QuizAnswerTrigger runs once for each record, with Trigger.new referencing only one record. When requests are coming in at the same time, in the before insert SOQL doesn't find the duplicate answer, since the duplicate hasn't been created yet. For example here's two records created at the same time. The trigger is run twice, and each time Trigger.new holds only one record.

Screen Shot 2019-09-25 at 9 42 27 AM

That's why I resorted to look into after insert on the QuizAnswerTrigger, but that ran into too many DML issues. Specifically the two tests on QuizSession that specifically stress tests with 150 players fail since we're calling DML on each answer, and there were 150 answers in each of those tests. Here's the complete after insert trigger

trigger QuizAnswerTrigger on Quiz_Answer__c (before insert, after insert) {
    AnswerService answerService = new AnswerService();
    List<Quiz_Answer__c> deleteThese = new List<Quiz_Answer__c>();
    for (Quiz_Answer__c answer : Trigger.new) {
        if (Trigger.isBefore) {
            // # of milliseconds since January 1, 1970, 00:00:00 GMT, ie. 1569261277045
            answer.Timestamp__c = Datetime.now().getTime();
        } else { // Trigger.isAfter
            try {
                // finds one record or throws error
                answerService.getFromPlayer(answer.Player__c, answer.Question__c);
            } catch (AbstractDataService.TooManyRecordFoundException e) {
                // DML on Trigger records is not allowed, hence 'create' record
                Quiz_Answer__c recordToDelete = new Quiz_Answer__c(
                    Id = answer.Id,
                    Player__c = answer.Player__c, 
                    Question__c = answer.Question__c, 
                    Answer__c = answer.Answer__c);
                deleteThese.add(recordToDelete);
            }            
        }
    }

    if (!deleteThese.isEmpty()) {
        delete deleteThese;
    }
}

Hence why I resorted to on Phase__c update on QuizSession trigger, which works.

pozil commented 5 years ago

Do you mean that the before insert trigger runs in parallel instances in case of multiple REST calls? I thought it was called in sequential more...

annyhe commented 5 years ago

The insert trigger is parallel with multiple REST calls. That's how double clicking results in records created at the same time (same to the seconds, at least).

pozil commented 5 years ago

Ok got it, thanks for clarifying.

Sorry if look stubborn but can you try one last thing? Try my before insert logic and try locking the player records. According to docs, that should also lock related records (answers):

When you perform a DML operation on one record, related records are locked in addition to the record in question.

annyhe commented 5 years ago

Done https://github.com/pozil/quiz-sfdx/pull/12. Please review @pozil

pozil commented 5 years ago

Thanks, merging and closing this issue.