Passing loggers between classes is bad and we should avoid this if
possible.
ValidationError#createLoggerSink required to pass in a logger just to
return it back to the caller wrapped in a lambda. Remove this method and
instead create the lambda directly in the caller. This brings us minimal
code duplication for formatting the message, but it's better than
passing the logger around.
Change-Id: I6b475de50fb2a1745c16ac424e0a95d358aaaf7d
Signed-off-by: Edwin Kempin <ekempin@google.com>
* Handle AuthenticationFailedException separately:
This exception is a subclass of AccountException that is thrown if the
user provides wrong credentials. For this exception we want to return
"Invalid username or password." as message to the client.
* Return a more general message for other AccountExceptions:
Likely they are not caused by invalid username or password since this
would cause a AuthenticationFailedException which we handle before.
* Increase log level to warning:
This is the log level that we use for these exceptions in other places
(e.g. ProjectBasicAuthFilter). Make it consistent.
* Log the stacktrace for AccountExceptions:
We do this everywhere else (e.g. ProjectBasicAuthFilter,
HttpLoginServlet). Make it consistent.
Change-Id: Ie34687d087b5a6cd102bf8cebd0f9830f54c9c1c
Signed-off-by: Edwin Kempin <ekempin@google.com>
Having the stacktrace when debugging this seems to be useful.
Change-Id: Ifa298ffb74eabdf0c6522c9831c533dd2ebd02eb
Signed-off-by: Edwin Kempin <ekempin@google.com>
There is no good reason why depending on //lib/truth should free
downstream libraries from needing a direct dep on //lib/guava or
//lib/junit. Exporting the deps was probably just oversight, since the
distinction between exports and runtime_deps in WORKSPACE is not
obvious, and there are probably other bad examples.
Change-Id: I1530106d642167eaa25222cc95c3996478728e15
We can just rely on the toString formatting of the List. This makes the
code more readable.
Change-Id: I98f0da6b5180198b9fcf0e45271e12e9ab182a52
Signed-off-by: Edwin Kempin <ekempin@google.com>
"log and throw" is considered a poor practice. The logging is uneeded
since the thrown exception will be logged somewhere else.
Change-Id: I82c210fe1e053a4ffd7a58b7fc7de6056f9875a7
Signed-off-by: Edwin Kempin <ekempin@google.com>
If a separate log statement is written for each line then the entries in
the log file can be interleaved with log entries from other threads,
which makes them less readable.
Change-Id: I038a0d8bc906746cc23b8f3bfb31d6c4d98b53c0
Signed-off-by: Edwin Kempin <ekempin@google.com>
Especially it's seems odd that copyStderrToLog logs the input on error
level but failing to do so results only in a debug log.
Change-Id: I52302be34e8f4a62639015acffe26d0a11a5c8da
Signed-off-by: Edwin Kempin <ekempin@google.com>
We used ProcessBuilder#toString() but ProcessBuilder doesn't implement
the toString() method, hence calling toString() would return the
instance identity (e.g. 'java.lang.ProcessBuilder@4488aabb'). Instead
include the command which is more useful.
This issue was detected by ErrorProne.
Change-Id: I780f21bbfeae8e7c1b3f4467d789bcf148293324
Signed-off-by: Edwin Kempin <ekempin@google.com>
We used to think adding this method to the top level is enough
and could avoid misuses. But it turns out we still need them in
other permission backend classes so that we only need to rescope
a user when necessary, which could provide better performance.
See I67b72b59 as an example.
Change-Id: I9ef30fb38315250fa63fb8b3d27e19e19d5e3e22
A colon is used as separator between the scheme and id. The logic in
ExternalId.Key#parse(String) that parses an external ID string looks for
the first occurrance of a colon to split the string. This means the
scheme must not contain any colon. Since a scheme is optional also the
id must not contain any colon.
We can't enforce this since internally at Google we violate this
assumption, but we also have our own parse logic and don't rely on the
parse logic in ExternalId.Key#parse(String).
Change-Id: I9a20776c6546eb09562f3da8f8e41506a74ba1e1
Signed-off-by: Edwin Kempin <ekempin@google.com>
* changes:
Move ReviewDb ProtobufCodecs to a common class
Add equals/hashCode to PatchSet and ChangeMessage
ChangeNotesState: Add a Builder
ChangeNotesState: Remove changeMessagesByPatchSet field
Submit: Remove code scanning for failed submission message
ChangeNotes: Remove fields that read from ChangeColumns
ChangeNotesState: Remove redundant revertOf field
Clarify ChangeNotesState private/WIP/started bits
ChangeNotesState: Remove some @Nullable annotations from create
Move ByteString bytes(int... ints) to test util
Add tests for fields declared in types with custom serializers
Convert OAuthTokenCache serialization to protobuf
Convert MergeabilityCacheImpl.Key serialization to protobuf
Simplify and clarify exception throwing of CacheSerializer
Some internal classes (namely PluginGuiceEnvironment and
PrivateInternals_DynamicTypes) rely on the fact that all DynamicMap
instances are DynamicMapImpl, which is not true if we have anonymous
subclasses of DynamicMap.
This is the case when #emptyMap is called, for instance from
BatchProgramModule.
Change-Id: I0e7d06bee2c13788e5c327fb70a60b7e86653d72
This allows us to replace them with custom bindings at Google.
Change-Id: I084ad42395882fd10d8cb9c8f3b842faac6dc52a
Signed-off-by: Edwin Kempin <ekempin@google.com>
This is a preparation to move the bindings for the REST API out of
GerritGlobalModule so that at Google we can replace them with custom
bindings.
Change-Id: I36b24206375ee0db4f14e8d50de7cd2721236747
Signed-off-by: Edwin Kempin <ekempin@google.com>
This makes the name consistent with the other REST API modules.
Change-Id: I5723685fddea36d1a4f777103b8f5bd27ac230ac
Signed-off-by: Edwin Kempin <ekempin@google.com>
This reverts commit 727d718b77d53e19ba14161ced72e0125087c220.
Reason for revert: We actually don't need this class to be public.
Change-Id: I25ceacb6e29f8e655bc5a28343d15b2e65aa3981
Earlier in this series as I was changing fields in this AutoValue class,
I kept messing up the ginormous constructor. It would have been nice to
have this builder to make editing the fields easier; better late than
never.
Change-Id: I4eaea9f8d4fa3920a7c5325729ce463f3ce13583
The only caller outside of tests that used this field was
Submit#getConflictMessage, which no longer exists.
Rename the remaining field from allChangeMessages to changeMessages,
since there is no longer overlap with another name.
Change-Id: I8f2846d7b21b41a6e0c934fd73ff87f6dc8e58d9
Back when we had separate SUBMITTED/MERGED states, the merge attempt
process would write a ChangeMessage from the server describing the
problem. This code was intended to pull out that message and report it
as the contents of a ResourceConflictException. The submit process no
longer works like this: errors during submission are supposed to be
thrown directly as IntegrationExceptions, and the code should not reach
this point. At any rate, on changes created in the past few years, this
code will never find a relevant message.
Change the case where change remains NEW after submission to be a 500, since it
reflects an actual server-side error.
Change-Id: Idbd27f7ed8daaf67318c391e9dbb1c8e02ed9e81
The point of ChangeColumns is that the contents are copied into the
relevant columns in the ChangeNotes's Change instance upon loading.
Callers should just be getting these fields from the Change instead;
there weren't any callers outside of tests doing the wrong thing anyway.
Change-Id: Ib6a592e4a5b28add9ed117dbcffb03ac7a507582
This is a @Column in Change and thus belongs in ChangeColumns. In
ChangeNotes#create it reuses the same value in both places, so they
should always be equal, modulo missing ChangeColumns, in which case the
field in ChangeNotesState should never be accessed.
Change-Id: I63eb5acdcd6c47ea2ceea7d830d5b5b6e4b28d2f
There were redundant fields in the ChangeNotesState and the contained
ChangeColumns. They properly belong in ChangeColumns, since they
correspond to actual @Columns in the Change class. Also in the Change
class, they are not nullable (they lack "notNull = false"), so remove
@Nullable in ChangeColumns as well.
A few callers were explicitly depending on the fields in
ChangeNotesState rather than in ChangeColumns; those are easily updated.
Note that the ChangeNotesState#create method always just passed in a
single value for each of these fields, which was then passed to two
different constructors, so they were always in sync.
The only time when the ChangeNotesState and ChangeColumns might
potentially be out of sync is when calling ChangeNotesState#empty, which
would pass a non-null value for these bits to the ChangeNotesState
constructor and leave the ChangeColumns null. However, this is only used
from ChangeNotes when NoteDb is disabled, in which case the
ChangeNotesState fields are never exposed to callers. In this case they
get these bits by calling the relevant methods on getChange(), which
will contain the ReviewDb change (and not have columns populated from
NoteDb via ChangeNotesState#copyColumnsTo).
Change-Id: Id4771f3e305417dc7f8d2040575d0c6746944946
These args were marked @Nullable, but the corresponding fields in
AutoValue were non-nullable, so remove @Nullable to make them
consistent.
For pastAssignees, any callers passing null would have already
encountered NPE due to the ImmutableSet#copyOf (and, if not that, the
check in the auto-generated constructor). For hashtags, push the logic
guarding against null into the one caller in
ChangeNotesParser#buildState, which already has similar logic to
construct its arguments.
Change-Id: Idc5123ccab4e1d21f932067d9ffff0f98afa5011
When the set of fields changed, it's a very good sign that something
needs to change in the serializer implementation, or at least bumping
the version number in the CacheMoule. Add a simple test utility that
maps declared field names to types, and list the expected types
explicitly in each of the relevant tests.
This simple mechanism can't catch everything, for example:
* If the serializer implementation changes for an existing type in an
incompatible way, without altering any fields. In this case, the
{key,value}Serializer in the CacheModule will change, so hopefully a
reviewer will notice and ask the author to bump the version as well.
* If semantics of individual fields change without changing the type,
such as making a field newly @Nullable.
But it's much better than nothing.
Change-Id: I51e60b83f9c3291d2f8f9ac19ce0192678211676
The serializer class lives in OAuthTokenCache.java so the extension
framework doesn't acquire a dep on the cache serialization code, which
currently lives in the server package.
Proto3 doesn't have optional fields, so we can't distinguish between
empty and null for the provider_id field (at least without adding a
separate has_provider_id field). Explicitly treat these the same in
OAuthToken itself, and clarify that other fields are not nullable.
Change-Id: I37bd47ac0c8491e5212fcec4885e063ae50d714d
