See [] for details, but basically, @FlowScope causes the Flow instance produced by flowProvider.get() in FlowRunner to be the same each time it's called, which leads to the instance being re-used when a transactional retry (e.g. for a ConcurrentModificationException) causes a flow to be attempted more than once. Flow is not meant to be re-used and certain flows fail at runtime when this happens, so the effect is that a CME now aborts most EPP requests, which is bad.
This is a bit of a hacky fix; finding a better one is tracked in []
== TESTING ==
This is very hard to test because there isn't really a clean way to trigger a CME from within a flow's execution without hardcoding in assumptions about what a given flow is doing when it runs, and we can't easily supply a custom Flow for testing while also exercising the Flow daggerization process (since this bug only appears due to the specific way that dagger constructs the Provider<Flow>). Ideally a fix would improve the testability here as well.
For now, I've manually tested this change by pasting code into FlowRunner that explicitly throws a ConcurrentModificationException after running the flow (similar to DryRunException), but only on the first transaction attempt. With @FlowScope on provideFlow(), this change reproduces the UnsupportedOperationException issue in many tests; once it's removed (i.e. with this CL submitted) the problem goes away.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=126226066
XjcToContactResourceConverter is, as it name suggests, an inverse of
ContactResourceToXjcConverter. This utility class is designed to
support the TLD data import feature.
EXTERNAL_REVIEW_URL=https://github.com/google/domain-registry/pull/19
GIT_AUTHOR=Wolfgang Meyers <wolfgang@donuts.co>
(With some minor changes by Ben McIlwain.)
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125985714
@Binds is a much more efficient way to delegate one binding to another in that:
* The @Binds method does not need to be invoked
* It does not generate an additional Provider implementation
* An additional Provider does does not need to be instantiated
Future changes to Dagger's component processor will add even further benefits.
More information: []
ATTENTION REVIEWERS: This migration is being done with global approval. Per-project approval is not required.
Tested:
TAP train for global presubmit queue
[]
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125980758
We've been using the very fragile newline-delimited legacy logging
statement in FlowRunner for ICANN reporting for a long time. While
this is bad in a few ways, the worst is that the parsing of this
logging statement is extremely fragile (e.g. adding/removing fields
can easily break the parsing). This is in fact part of what broke the
ExportLogsServlet parsing last fall ([] and forced us to
recover by manually parsing the log statement (and its XML) in
BigQuery. It also broke again in [] where we were relying
on matching the logging classname, since matching on 'EPP Command'
was considered insufficiently narrow.
This introduces a new JSON-format logging statement to FlowRunner
that fixes both of these problems:
1) it replaces the newline-delimited "format" with a JSON-based
format, so that we can add new fields much more easily and
reliably support logging more structured data
2) it replaces the short 'EPP Command' signature with a much more
targeted 'EPP-REPORTING-LOG-SIGNATURE' signature so that we can
use that alone for matching, rather than relying on the class
name in the log message
What this doesn't fix is the fact that we still need to parse the
XML in BigQuery; we should fix this by logging the parts of the XML
that ICANN reporting needs explicitly, but that'll be a subsequent
change, since while the existing approach is gross, it's actually
much less fragile than just matching the log statement itself.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125902976
This cleanups up the toString() methods of all implementations of
these interfaces, as pre-work for adding tests against the legacy
logging statement in FlowRunner used for ICANN reporting, so that we
can validate against any changes to that log statement in the future.
It removes system hash codes since those aren't really safe to rely on
in test code and they really don't help with debugging anyway. It
also standardizes SessionMetadata.toString() a bit and regroups
methods on that interface so all the getters are together.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125686039
This changelist makes the newly added v1_2_0 the default and fixes incompatibilities
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125609347
If the enclosing module can be abstract, pull @Multibinds methods up into it; otherwise, replace the @Multibindings interface with a @Module and include it into the enclosing module.
Tested:
TAP --sample for global presubmit queue
[]
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125589112
All [] paths relating to soy are encompassed by @io_bazel_rules_closure[]
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125498324
This is more efficient (we were constructing all the
providers and the map anew for every flow) and prettier.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125479328
Daggerizes all of the EPP flows. This does not change anything yet
about the flows themselves, just how they are invoked, but after
this CL it's safe to @Inject things into flow classes.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125382478
Despite the comment, DomainAllocateFlow is absolutely registered in
FlowPicker. It gets picked if there's a domain create epp command that
also specifies the allocate extension. Remove the explicit setting of
flowClass, and remove two tests that now fail because DomainCreateFlow
gets loaded - which is the desired behavior.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125339191
The "SessionSource" has nothing to do with sessions (and it's often
used in sessionless contexts). What it does indicate is the endpoint
used to make the request.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125295224
Superuser should only be settable via the tool (see []
which is merged in here but not diffbased, and which removes
the implicit superuser for CharlestonRoad). It is a property
of the request, not of the session (there are no sessions in the tool).
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125204707
dryRun is only available via the (sessionless!) tool, and is not
a property of the session.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125203026
TransportCredentials are per-request, not per-session, and
there's no reason to carry them within SessionMetadata.
While I'm in here, get rid of "null" credentials.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125202213
We need a flavor that accepts a list in order to set this from data returned
from the pricing system.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125078913
Add a method to get the next transition time so that we can return the expiry
date along with the EAP fee for a given time.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125076484
Add a flag to CreateTldCommand to allow us to set the EAP fee schedule for the
registry.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=125068579
Verify that the currency of all entries in the EAP fee schedule map matches
that of the registry object itself.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=124967612
The saveAndUpdateEntries() method was always saving entries under the current entity's revisionKey, and then always deleting them under the old entity's revisionKey - even if the revisionKeys were the same (i.e., we didn't change any entry content in the list, in which case the builder's build() method doesn't create a new revisionKey).
This would have caused the existing entries to be unnecessarily resaved, and (worse) then caused those same existing entries to all be deleted.
This fixes the bug by only resaving entries when the revisionKey has changed.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=124873871
- Update DnsUpdateWriter to load nameservers names using
DomainBase.loadNameserverFullyQualifiedHostNames() to be consistent with
other uses.
- Pass "whole entity" to helper methods to simplify code.
- Cleanup damage from prior rename of DnsMessageTransport.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=124854517
Just an old patch that I had lying around and never mailed out. Seemed like it could be worth having checked in.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=124853504
