PHPLIB-683 Test against Proxy as a Mongos #854
Conversation
| private static function appendAuthenticationOptions(array $options): array | ||
| { | ||
| if (isset($options['username']) || isset($options['password'])) { | ||
| return $options; |
There was a problem hiding this comment.
Perhaps it makes sense to raise an exception here, or maybe only if the options are later specified and would try to append themselves?
I reckon there isn't a use case where we'd actually want MONGODB_USERNAME or MONGODB_PASSWORD to override anything, apart from the previous edge case I mentioned about the connection string already having auth credentials.
There was a problem hiding this comment.
This method is called automatically, and only appends default options if the URI being given doesn't already contain them. This allows us to specifically test with invalid credentials on clusters with auth enabled, which would otherwise be impossible (either due to the exception we'd raise, or because we'd always inject the credentials from the environment).
There was a problem hiding this comment.
I'm still a bit confused.
only appends default options if the URI being given doesn't already contain them
Is "URI" referring to the connection string or the options array? If the connection string, I don't see the logic that avoids calling appendAuthenticationOptions when credentials are already present.
Alternatively, are you intentionally expecting the array options to override credentials in the connection string? Perhaps that's what b5212a0 is clarifying?
There was a problem hiding this comment.
Sorry, I should've clarified. Yes, this does not catch credentials sent through the connection string, which we currently don't do in tests. This ensures that any client created with URI options that set specific credentials (e.g. to test failing authentication) does not get correct credentials injected.
|
|
||
| if ($this->isServerRequirementSatisifed($minServerVersion, $maxServerVersion, $topologies)) { | ||
| if ($this->isServerRequirementSatisifed($minServerVersion, $maxServerVersion, $topologies, $serverlessMode)) { |
There was a problem hiding this comment.
Should serverless pre-empt any requirement checking for server version? Feel free to point me to some language in the spec test READMEs that say as much. I don't recall the requirements being mutually exclusive, though.
There was a problem hiding this comment.
Theoretically, the check should pre-empt any other checks, as it's entirely based on the environment variable being set or not. That said, the unified test format currently prescribes that the requirements MUST be checked in the following order:
- minServerVersion
- maxServerVersion
- topologies
- serverless
- serverParameters
- auth
I don't think this order is efficient or even necessary, so I'm happy to have the spec updated to suggest a more efficient order without mandating it (to avoid unnecessary downstream changes in drivers)
There was a problem hiding this comment.
There was a problem hiding this comment.
DRIVERS-1885 is now resolved if you'd like to restructure this.
There was a problem hiding this comment.
I'd defer that work to a separate PR in an interest to get this merged.
Synced with mongodb/specifications#1057 Note: aggregate tests are intentionally excluded since those will be synced by mongodb#854
Synced with mongodb/specifications#1057 Note: aggregate tests are intentionally excluded since those will be synced by mongodb#854
Synced with mongodb/specifications#1057 Note: aggregate tests are intentionally excluded since those will be synced by mongodb#854
Synced with mongodb/specifications@59a5dad Note: aggregate tests are intentionally excluded since those will be synced by mongodb#854
…pported by the server (#859) * Sync unified CRUD spec tests Synced with mongodb/specifications@59a5dad Note: aggregate tests are intentionally excluded since those will be synced by #854 * PHPLIB-711: Unwrap BulkWriteException when evaluating isClientError * Client-side errors for hint with update, delete, and findAndModify The CRUD spec requires a client-side error if hint is used and the server would not otherwise raise an error for an unsupported option. This was already being done, but this commit renames an internal constant and adds functional tests. A client-side error is also required if hint is used with an unacknowledged write concern and the server does not support hint for the operation (regardless of error reporting for unsupported options). This was previously done only for FindAndModify; however, it failed to detect some acknowledged write concerns (PHPLIB-712). That issue has been fixed and the commit additionally adds this behavior for update and delete operations.
The tests for transactions are the same for the classic and convenient APIs. Since the serverless spec excludes the convenient API from testing, we need to split this so we can only target the classic API for testing.
PHPLIB-683
I've decided to vendor in the setup scripts for better local testing. For now this works, but I'd like to figure out a better alternative in the long-term to avoid having to duplicate changes made in drivers-evergreen-tools.