Skip to content

Rails: Enforce that all fields in passed from agent in reconciliation request are non-null

MR: Pending

Description

Enforce that all fields in passed from agent in reconciliation request are non-null.

Note that this will need to be done in coordination with Explicitly set fields while responding from age... (#409133) - see details in this comment.

This issue has been created as a followup to this comment thread:

See technical details below.

Acceptance Criteria

  • Coordinate changes, versioning, backward compatibility and rollout with changes on the agent side. See Explicitly set fields while responding from age... (#409133) and this comment for details.
  • Ensure that namespace is always non-null when parsing agent info, and raise an exception if it is not.
  • Enforce non-null field values for all other fields, to eliminate nulls from the reconcilation data structures. If we are ever passing an enumerable, i.e. array or hash type, it should be represented as an empty array/hash, not as a nil.
  • If there are any atomic (i.e. non-enumerable) type where the absence of a value can only be reflected as nil, discuss with team whether this can be changed somehow to make it a required field.

Technical Requirements

Discussion from original thread:

Chad Woolley

Thinking about this spec more.

Why should we ever expect the namespace to be nil in the agent payload?

The agent should always know the namespace, since it was provided by rails, and thus be able to echo it back.

Therefore, shouldn't we assume this field is required, and having it not set in the agent payload should indicate a bug in the agent/rails communication protocol, and therefore raise an exception in the reconcile request?

Yes, an exception, not an error where we process the rest of the workspaces, because communication protocol bugs should required to be addressed, and not ignorable as just a log message.

Vishal Tak

Yes, we should be doing a hard-check to see the namespace is present in the agent request or not.

Chad Woolley

Can we make a follow-up issue to reflect this investigation and change?

And perhaps it can include a type review of other fields, to eliminate nulls from the reconcilation data structures where possible?

E.g. if we are ever passing an enumerable, i.e. array or hash type, it should be represented as an empty array/hash, not as a nil.

This will:

  1. Allow us to tighten up the type safety by enforcing non-null fields (i.e. don't need <NilClass|Array>)
  2. Make defining current and future schemas easier because we don't have to account for these fields being optional
  3. It makes the code simpler, as it doesn't have to account for the possibility of nulls.

Only when it's an atomic (i.e. non-enumerable) type should the absence of a value be reflected as nil.

Edited by Chad Woolley