Commit graph

1175 commits

Author SHA1 Message Date
ctingue
443e260f91 Disallow renaming of external hosts
EPP host spec section 3.2.5 requires that attempts to update hosts that are linked to different registrars must fail with error code 2305. This is complicated to do, as linked status is eventually consistent, and even more painful when checking links to those of different registrars.

This change forbids external-to-anything renames entirely.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=150336754
2017-03-21 15:24:28 -04:00
nickfelt
e1c9395749 Simplify blocking of renews for domains in pending transfer
This tweaks the logic that prohibits domain renews during pending transfers to just use the regular verifyNoDisallowedStatuses() check instead of a special check on TransferData with a custom exception.  This is simpler and produces a better error message: we get "Operation disallowed by status: pendingTransfer" instead of "Object with given ID (foo.com) already has a pending transfer" (which is intended for use when denying a transfer request for an object already being transferred, not for this case).

For the record, we originally prohibited renews for domains in pending transfer because there's no good reason to do such a renew: b/12533793.  But in fact our transfer server-approve logic relies heavily on this behavior, because otherwise the domain's expiration time computed in cloneProjectedAtTime() will reflect the transfer year added to the post-renew expiration time, whereas all the transfer server approve entities (e.g. new autorenew billing event) will reflect the pre-renew expiration time at the moment the transfer was requested.  As such, it would be quite difficult to ever support a renew during pending transfer, since it would need to change many fields within the transfer server approve entities.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=150325501
2017-03-21 15:21:28 -04:00
nickfelt
6a8b25360c Use StatusValue XML names in EPP error messages
This changes ResourceStatusProhibitsOperationException so that we print out the list of StatusValues using their XML names rather than the literal enum name, i.e. we use "pendingDelete" rather than "PENDING_DELETE".

This seems more correct given that EPP clients will be used to seeing the status values in the XML representation, and it also matches the existing ResourceHasClientUpdateProhibitedException that hardcodes "clientUpdateProhibited":
http://[]/third_party/java_src/gtld/java/google/registry/flows/exceptions/ResourceHasClientUpdateProhibitedException.java?l=22&rcl=146111211

Also reorganized related test methods and added some missing tests, including for ContactTransferRequestFlow which previously had none.  I also renamed the "clientProhibitedStatusValue" tests to instead say "statusValueNotClientSettable" to be clearer about what's being tested, and that it's not related to the "clientXXProhibited" statuses.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=150248562
2017-03-21 15:14:02 -04:00
mountford
e60c01c2f7 Fix incorrect pluralization in reserved list metric name.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=150241148
2017-03-21 15:12:35 -04:00
nickfelt
f28104ad03 Restrict domain transfer periods to 1 year
It turns out that this ICANN policy appears to prohibit transfers with
registration extensions other than 1 year (section A.8):
https://www.icann.org/resources/pages/policy-2012-03-07-en

This is backed up by the practical fact that we've never seen a registrar
request a transfer for any period other than one year.

And removing the support for multi-year transfers vastly simplifies
transfer logic and eliminates a bunch of annoying corner cases.  Users
still can achieve the same thing by doing a 1-year transfer plus a
manual renewal afterwards for the remainder of the desired extension.

This change leaves in place lots of infrastructure to support multi-year
transfers that is now obsolete (e.g. TransferData.extendedRegistrationYears).
This should all be cleaned up, but it's a lower priority than fixing the
gap itself and insulating ourselves against needing to handle any real
multi-year transfer case.  Once this CL goes in, we can start ignoring
extendedRegistrationYears entirely because it'll always be 1 year, which
makes the cleanup process easier.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=150212864
2017-03-21 15:11:07 -04:00
guyben
e6a5083b55 Rename .toString and .equals to be less confusing.
There will now be "resultToString" and "resultEquals" to stringify / compare
the method results.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=150135475
2017-03-21 15:06:44 -04:00
mcilwain
3a13a4d6b6 Add monitoring/metrics/contrib package to Nomulus release
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=150007228
2017-03-21 14:59:31 -04:00
jianglai
c426a80563 Add a new reservation type to support nameserver restrictions
A new field (allowedNameservers) is added to ReservedListEntry that stores the allow nameservers for the label. The field itself is a comma separated string, but the actual lines within a reserved list file (from which the field is parsed) uses colon to separate nameservers, to avoid conflicting with the commas used as primary separators in a CSV file.

Combined with upcoming update(s) that enables locking down an entire TLD to only delegate domains with a nameserver restricted reservation type, this change will enable us to restrict domain delegation to nameservers specifically specified in the allowed nameservers list, in order to prevent malicious delegation in case the registrar for a brand TLD is compromised.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149989330
2017-03-21 14:56:31 -04:00
mountford
5253f6fd6b Define Stackdriver metrics for premium and reserved lists
This CL defines metrics for both premium and reserved lists, but actually uses only the reserved list metrics. The premium list metrics will be used in a future CL.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149982639
2017-03-21 14:55:00 -04:00
shikhman
5779cc988b Refactor RdeKeyringModule into FakeKeyringModule
A fake Keyring implementation is needed to test Keyring related tools now, so
I've refactored the fake RdeKeyring to support all Keyring API methods and
moved it to the common testing package.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149977968
2017-03-21 14:53:25 -04:00
nickfelt
2353bcd8c5 Cut over to generating new HMAC-based XSRF tokens
This is the second step of migrating to our new XSRF token format.  The
first step ([] made validate() start accepting new tokens
(basically, dual-read).  This step cuts over our "writing" to write the
new token format.  The third and final step will drop support for
validating the old token format (back to single-read).  We'll do that
in a subsequent push so that we don't invalidate all the current XSRF
tokens that people might have in their browsers.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149790648
2017-03-13 11:22:56 -04:00
jianglai
ebcdae7361 Return all applicable reserved list entries associated with a label
Instead of only returning the most severe one, return all applicable ones. This is because the reserved list has grown to a list of types that are not strictly comparable but orthogonal to each other. We can no longer depend on the fact that the most severe type incorporates all properties of those beneath it. Therefore returning all of them and treat them one by one in the calling site is the correct behavior.

Due to constraint imposed in eppcom.xsd, during domain checks the response can only contain a reservation reason of fewer than 32 characters, therefore we are returning the message for the type with highest severity, in case of multiple reservation types for a label.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149776106
2017-03-13 11:22:56 -04:00
cgoldfeder
9174855a47 Remove the ofy().load() inside of HostResource.cloneProjectedAtTime
In fact, completely eviscerate cloneProjectedAtTime (to be removed in
a followup CL) in favor of doing the projection of transfers and the
loading of values from the superordinate domain at call sites. This
is one of the issues that blocked the memcache audit work, since the
load inside of cloneProjectedAtTime could not be controlled by the
caller.

Note: fixed a minor bug where a subordinate host created after its superordinate domain was last transferred should have lastTransferTime==null but was previously reporting the domain's lastTransferTime.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149769125
2017-03-13 11:22:55 -04:00
mountford
1f000b94e6 Set correct auth settings for all actions
A test has been added to RequestHandlerTest, making sure that, while we merely log errors for the time being, the correct dummy AuthResult is being created.

Most actions use the default settings, which have been changed to INTERNAL / APP / IGNORED. Actions with non-default settings are:

INTERNAL/NONE/PUBLIC (non-auth public endpoints)

CheckApiAction
WhoisHttpServer
Rdap*Action

INTERNAL,API/APP/ADMIN (things currently protected by web.xml)

EppTlsAction
EppToolAction
CreateGroupsAction
CreatePremiumListAction
DeleteEntityAction
List*sAction
UpdatePremiumListAction
VerifyOteAction
WhoisServer

INTERNAL,API,LEGACY/USER/PUBLIC (registrar console)

RegistrarPaymentAction
RegistrarPaymentSetupAction
RegistrarSettingsAction
EppConsoleAction

INTERNAL,API,LEGACY/NONE/PUBLIC (registrar console main page)

ConsoleUiAction

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149761652
2017-03-13 11:22:55 -04:00
Ben McIlwain
f5e76868f0 Add generic implementation comparator
This is an alternative to the "manually constructed" version in []

This generator recieves two objects implementing some interface. It returns a new implementation that exactly duplicates the behavior (return values, exceptions etc.) of the first ("original") object, but also logs any differences (in either return value or thrown exception) from the second object.

Nothing in the second object's execution will affect the results - not even thrown exceptions - it's only used for logging any differences.

This is useful when switching backend architecture for the some interface (say... from keystore to datastore+KMS for the Keyring interface :)) as we can make sure everything was OK for some extended period of real data before doing the switch.

This is better than just comparing the outputs once because having a completely different backend out of our control can result in "glitches" that are rare but relevant: maybe the new backend has lower availability, or sometimes fails and gives wrong results, or... well, that's it.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149706165
2017-03-13 11:22:55 -04:00
mcilwain
ad840ad4d6 Better handle syncing contacts when creating RegistrarContacts
This also adds a test that should've been there already.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149583500
2017-03-13 11:22:55 -04:00
mcilwain
27b9244126 Fix non-determinism in RegistryTest duplicate auth code test
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149576311
2017-03-13 11:22:55 -04:00
mcilwain
815dae2749 Refactor WhoisReader.readCommand() method to take a now param
This obviates the need for the use of @AutoFactory.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149570411
2017-03-13 10:45:40 -04:00
shikhman
9eddbe2b6e Add test which verifies that all commands have descriptions
Besides being good practice, this works around a bug in JCommander 1.48, which we are stuck with for now
since newer versions of the library are Java 8 only:
https://github.com/cbeust/jcommander/issues/270

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149565357
2017-03-13 10:44:12 -04:00
mcilwain
d2ca4b7234 Refactor main loop of MapreduceEntityCleanupAction
This also tightens up some error-checking conditions.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149552065
2017-03-13 10:38:21 -04:00
mcilwain
3fcb564251 Record metrics for WHOIS commands
Note that this does not write out metrics for invocations of the
nomulus tool.

This requires a slight refactoring of the existing WhoisResponse
interface so as to also support returning the number of results found
by the WHOIS query.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149461208
2017-03-13 10:30:53 -04:00
mcilwain
ce4f3c0d56 Don't allow setting reserved lists with conflicting auth codes
This is an error condition that will soon throw an exception when
attempting to register the domain name, so it's good to let the registry
operator know of the error when it is first introduced.

Unfortunately there's still a backdoor that allows duplicate labels
that's harder to protect against (that this commit doesn't cover): the
case where reserved lists are already applied to a TLD, then one of the
reserved lists is updated to add another auth code, which then conflicts
with one on a different reserved list.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149443007
2017-03-13 10:27:51 -04:00
mountford
5d4287a375 Add mapreduce cleanup action
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149432516
2017-03-13 10:24:57 -04:00
mcilwain
bd7db61606 Use Dagger to @Inject WhoisReader instances
This is a precursor to adding metrics to WHOIS queries (as I'd like
to be able to @Inject the metrics builders).

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149418018
2017-03-07 14:02:46 -05:00
Ben McIlwain
439742fd11 Use Immutable types for static final fields
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149260513
2017-03-07 13:52:19 -05:00
Ben McIlwain
2bba5660c2 Use Immutable types for static final fields
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149172966
2017-03-07 13:50:53 -05:00
mmuller
5614760d53 Implement login/logout commands
Refactor the auth code into its own dagger module, add tests and use the new interfaces to implement the login and logout commands.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149108266
2017-03-07 13:45:11 -05:00
Ben McIlwain
80f0910899 Use Immutable types for static final fields
Replace constants (static final CONSTANT_CASE) declaration type which use the general collection interface (e.g. List) with an immutable type (e.g. ImmutableList).

For constant field declarations, you should use the immutable type (such as ImmutableList) instead of the general collection interface type (such as List). This communicates to your callers important semantic guarantees ([]

For more info, see: []

Cleanup change automatically generated by javacflume/refactory
Refactoring: //third_party/java_src/error_prone/project/core/src/main/java/com/google/errorprone/bugpatterns:MutableConstantField_refactoring
Tested:
    TAP --sample for global presubmit queue
    []

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149061247
2017-03-07 13:43:43 -05:00
mountford
ee2bd594c8 Change new authorization logic to log a warning rather than rejecting the request
This is the first step in rolling out the changes so that we can check via logging whether turning on the logic would reject anything it should not.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149050878
2017-03-07 13:42:16 -05:00
jianglai
b1b69656bd Remove reference to TldSpecificLogic
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149030846
2017-03-07 13:40:48 -05:00
nickfelt
3a7f67b7f3 Fix DomainTransferRequestFlow to correctly cancel autorenew graces
This fixes longstanding bug b/19430703 in which domain transfers that were
server-approved would only handle the autorenew grace period correctly if
the autorenew grace period was going to start within the transfer window.
If the autorenew grace period was already active (e.g. the domain had
recently autorenewed, before the transfer was requested), the logic would
miss it, even if it was going to be active throughout the transfer window
(i.e. it would still be active at the server-approval time).

When the autorenew grace period is active at the time a transfer is approved
(whether by the server or explicitly via DomainTransferApproveFlow), the
correct behavior is to essentially "cancel" the autorenew - the losing registrar
receives a refund for the autorenew charge, and the gaining registrar's transfer
extended registration years are applied to the expiration time as it was prior
to that autorenew.  The way we implement this is that we just have the transfer
essentially "subsume" the autorenew - we deduct 1 year from the transfer's
extended registration years before extending the registration period from what
the expiration time is post-autorenew at the moment of transfer approval.

See b/19430703#comment17 for details on the policy justification; the only real
ICANN document about this is https://www.icann.org/news/advisory-2002-06-06-en,
but registrars informally document in many places that transfers will trigger
autorenew grace, e.g. see https://support.google.com/domains/answer/3251236

There are still a few parts of this bug that remain unfixed:

  1) RdeDomainImportAction repeats a lot of logic when handling imported domains
     that are in pending transfer, so it will also need to address this case in
     some way, but the policy choices there are unclear so I'm waiting until we
     know more about RDE import goals to figure out how to fix that.

  2) Behavior at the millisecond edge cases is inconsistent - specifically, for
     the case where a transfer is requested such that the automatic transfer
     time is exactly the domain's expiration time (down to the millisecond),
     the correct behavior is a little unclear and this CL for now ignores this
     issue in favor of getting a fix for 99.999% of the issue into prod.  See
     newly created b/35881941 for the gory details.

Also, there are parts of this bug that will be fixed as parts of either
b/25084229 (transfer exDate computations) or b/35110537 (disallowing transfers
with extended registration years other than 1), both of which are less pressing.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149024269
2017-03-07 13:39:15 -05:00
nickfelt
f663f00251 Add test and commentary for "phantom autorenew" in pendingDelete
This documents some slightly spooky behavior around domains that have an expiration time within their pendingDelete window (meaning the whole period from DomainDeleteFlow running to the actual deletionTime, not just the 5-day pendingDelete grace period).  They will experience an autorenew in terms of expiration time and grace period status due to cloneProjectedAtTime(), but without the usual artifacts of an autorenew (billing event and poll message).

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149019980
2017-03-07 13:37:40 -05:00
Ben McIlwain
f61f1373df Improve error messages for LogsSubject
Currently, hasLogAtLevelWithMessage gave a very generic "no log at level with
message was found" error on failure. The user didn't know if there were no logs
at all, or if there was a small typo in the log etc.

Using the "regular" Truth assertions gives more informative errors in cleaner code.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=149007204
2017-03-07 13:33:20 -05:00
nickfelt
2e969d6ed1 Change XsrfTokenManager to support new HMAC token format
This follows up on Brian's work to transition not just to a new format
with an empty scope value, but instead to replace the existing format
entirely with a new one that:

  1) includes a version number to support future format migrations
  2) doesn't include a field for the scope at all, since scoping the
     tokens adds no real security benefit and just makes verification
     more difficult
  3) replaces the raw SHA-256 hash with a SHA-256 HMAC instead, as a
     best practice to avoid length-extension attacks [1], even though
     in our particular case they would only be able to extend the
     timestamp and would thus be relatively innocuous

The new format will be produced by calling generateToken(), and the
scope-accepting version is renamed to generateLegacyToken() in addition
to its existing deprecation, for maximum clarity.

I changed the validateToken() logic to stop accepting a scope entirely;
when validating a legacy-style token, we'll test it against the two
existing legacy scope values ("admin" and "console") and accept it if
it matches either one.

Note that this means the xsrfScope parameter in @Action is now wholly
obsolete; I'll remove it in a follow-up to avoid bringing extra files
into this CL.

After this CL hits production, the next one will replace all calls to
generateLegacyToken() with generateToken().  Once that CL is deployed,
the last step will be removing the legacy fallback in validateToken().

[1] See https://en.wikipedia.org/wiki/Length_extension_attack

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148936805
2017-03-07 13:31:46 -05:00
nickfelt
499f1e7dbc Rewrite ServerSecret caching and accessor logic
I'm working on some changes to XsrfTokenManager (b/35388772) and ServerSecret
was crufty enough that I ended up rewriting it.  Now it uses a LoadingCache
with a transaction instead of needlessly race-condition-y static init logic.

It also now supports retrieving its value as either a UUID (the old format
used by XsrfTokenManager) or a byte[].  The latter is more flexible and can
be directly used with HMAC which the new XsrfTokenManager format will employ.

And lastly, I added tests.  In addition, I tested this code on alpha and
verified appropriate operation (XSRF tokens still work from the console and
from regtool; if you remove ServerSecret from datastore and memcache, it
persists a new one).

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148931620
2017-03-07 13:30:18 -05:00
nickfelt
c56959b62b Add new DomainResource.getGracePeriodsOfType() method
This adds a new method which will be used in an upcoming CL affecting domain
transfer logic.  It also removes two older methods that are unused (they were
originally going to be used for TLD-specific logic which is now obsolete).

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148928965
2017-03-07 13:28:49 -05:00
shikhman
51a613bca6 Fix bugs in KmsConnectionImpl
A few errors emerged when doing an integration test against the actual API. I've updated the unit tests to reflect the correct behavior.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148793856
2017-03-07 13:25:51 -05:00
nickfelt
726e925b4a Refactor a few new XsrfTokenManager methods
Followup to comments on []

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148792464
2017-03-07 13:24:21 -05:00
nickfelt
93b5cfb71b Make RdeUploadActionTest compatible with IPv6-only mode
This fixes an issue where RdeUploadActionTest relies on connecting to a local dummy SFTP server via the IPv4 loopback (127.0.0.1), which causes the test to fail on machines that only support IPv6.  This updates the test to use just "localhost" instead.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148698178
2017-03-07 13:19:56 -05:00
mountford
c7a62e9b98 Add XSRF protection to legacy authentication mechanism
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148689952
2017-03-07 13:18:04 -05:00
mcilwain
90114858fa Perform minor refactors on premium list code
Principally, this moves a load method into DatastoreHelper that is now
only used by tests.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148649087
2017-02-27 11:30:45 -05:00
mcilwain
ea4e471c04 Move premium list static helper methods into their own class
It was kind of messy having all of that logic living alongside the
entities themselves.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148498024
2017-02-27 11:30:45 -05:00
shikhman
388dd1055e Abstract KMS code with KmsConnection and create a fake KmsConnection
This simplifies the tests for KmsKeyring and KmsUpdater.

This is a followup to []

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148496758
2017-02-27 11:30:45 -05:00
Wolfgang Meyers
9f90597691 Handle pending transfers on domain imports from RDE deposits
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148496597
2017-02-27 11:30:40 -05:00
mcilwain
3ca9bb6aeb Read from bloom filter for premium pricing checks
This also cleans up the PremiumList API so that it only has one
method for checking premium prices, which is by TLD, rather than two.

I will be refactoring a lot of the static methods currently residing in
the PremiumList class into a separate utils class, but I don't want to
include too many changes in this one CL.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148475345
2017-02-27 11:22:21 -05:00
mountford
3ac74fa449 Add more tests to new authentication framework
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148459400
2017-02-27 11:19:27 -05:00
mcilwain
dd400f30f5 Don't allow duplicates in premium/reserved lists
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148458642
2017-02-27 11:17:58 -05:00
nickfelt
ea3a8dfa9d Make Router reject classes with no @Action-returning methods
This provides a safeguard against using TypeInstantiator to resolve the component class, where if resolution is done incorrectly, you end up with java.lang.Object.  Formerly, that would have "succeeded" in generating a Router for Object, which of course has no methods that return @Action classes.  Such a router is pretty useless, so it's better to make Router stricter and have it fail if you give it such a class by accident.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148353224
2017-02-27 11:16:30 -05:00
nickfelt
d8ce444251 Fix stale doPost references in servlet test method names
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148352385
2017-02-27 11:15:01 -05:00
nickfelt
1e7fc4d64d Remove Builder type param on RequestComponentBuilder/RequestHandler
It turns out this type parameter was never necessary.  A builder only needs the reflexive second type parameter when you want to have a builder inheritance hierarchy where the descendant builders have methods that the ancestor builder doesn't.  In that case, the type param enables the ancestor builder's setter methods to automatically return the correct derived type, so that if you start with a derived builder, you can call a setter method inherited from an ancestor and then continue the chain with setters from the derived builder (e.g. new ContactResource.Builder().setCreationTime(now).setContactId(), which otherwise would have returned an EppResource.Builder from setCreationTime(), at which point the call to setContactId() would not compile).

Even then, it's not strictly necessary to use the type parameter, since you could instead just have each derived type override every inherited method to specify itself as the return type.  But that would be a lot of extra boilerplate and brittleness.

Anyway, in this case, there is a builder hierarchy, but RequestComponentBuilder specifies all the methods that we're ever going to want on our builders, so there's never any need to be able to call specific derived builder methods.  We only even need the individual builder classes so that Dagger can generate them separately for each component.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148269178
2017-02-27 11:12:07 -05:00