divviup / divviup/janus

Wrong error type for collection with a fixed size current batch query

Open
#989 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
70
Forks
16
Avg merge
4h 36m
Merged PRs (30d)
30

Description

Currently, if a collect request with a current_batch query is made when there are not enough reports to form a new batch, Janus will return urn:ietf:params:ppm:dap:error:batchInvalid. I think this is incorrect for the following reason: the batchInvalid error type only arises in section 4.5.6, Batch Validation. The relevant paragraph says that this error is returned if the query type's "boundary check" fails. Section 4.5.6.2.1 says that a by_batch_id query can fail the boundary check if the leader or helper does not recognize the given batch ID. It does not mention current_batch queries at all, so I don't think they can fail this check. I would instead interpret this situation as failing the size check (since there are fewer than min_batch_size reports in all possible current batches) which should result in urn:ietf:params:ppm:dap:error:invalidBatchSize.

I think there are two things for us to do here:

  • Swap out the error code we apply when transforming None from get_filled_outstanding_batch().
  • Make <FixedSize as CollectableQueryType>::validate_collect_identifier() or <FixedSize as CollectableQueryType>::batch_identifier_for_query() check the database for the existence of the batch identifier inside a by_batch_id query, so that we properly implement that boundary check.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by tracing where get_filled_outstanding_batch() returns None and where FixedSize as CollectableQueryType implements validate_collect_identifier() and batch_identifier_for_query(). Compare current_batch handling with the DAP batch-validation rules, then verify that insufficient reports use invalidBatchSize while by_batch_id identifiers receive the appropriate boundary check.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend-api-design, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.