[EXTERNAL] Optional dependency injection fails if the optional dependency can't be built

đź’» Development
,
1

Hello,

The following pull request allegedly added support for optional dependency injections: https://github.com/nextcloud/server/pull/41265

However I can’t get it to work, my Controller cannot be instantiated if one of its optional dependencies cannot be instantiated even though I marked them as optional.

My flow:

  • I have a class PageController that requests an optional ?PreferenceService $preferenceService injection
  • PreferenceService requests string $userId (not optional)
  • The user is not connected so $userId cannot be injected (it’s null)

What I would expect is that PreferenceService can’t be built because $userId cannot be null, however PageController would be instantiated without issues because although PreferenceService couldn’t be built, it’s an optional dependency.

Here is my attempt: https://github.com/devnoname120/epubviewer/commit/89a76c25ed25e5363667f8c570d9098745ce943e

What I see is this instead:

{
"reqId": "SA3HE70WauL60defOmaI",
"level": 3,
"time": "2025-02-26T01:17:09+00:00",
"remoteAddr": "192.168.215.1",
"user": "--",
"app": "index",
"method": "GET",
"url": "/index.php/apps/epubviewer/?file=%2Findex.php%2Fs%2FrsQgKNjyNGqgYEP%2Fdownload&type=application%2Fepub%2Bzip",
"message": "OCA\Epubviewer\Db\BookmarkMapper::__construct(): Argument #2 ($userId) must be of type string, null given",
"userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/133.0.0.0 Safari/537.36",
"version": "30.0.6.2",
"exception": {
"Exception": "TypeError",
"Message": "OCA\Epubviewer\Db\BookmarkMapper::__construct(): Argument #2 ($userId) must be of type string, null given",
"Code": 0,
"Trace": [
{
"function": "__construct",
"class": "OCA\Epubviewer\Db\BookmarkMapper",
"type": "->",
"args": [
{
"__class__": "OC\DB\ConnectionAdapter"
},
null,
{
"__class__": "OCA\Epubviewer\Utility\Time"
}
]
},
{
"file": "/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php",
"line": 98,
"function": "newInstanceArgs",
"class": "ReflectionClass",
"type": "->",
"args": [
[
{
"__class__": "OC\DB\ConnectionAdapter"
},
null,
{
"__class__": "OCA\Epubviewer\Utility\Time"
}
]
]
},
{
"file": "/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php",
"line": 106,
"function": "buildClass",
"class": "OC\AppFramework\Utility\SimpleContainer",
"type": "->",
"args": [
{
"__class__": "ReflectionClass",
"name": "OCA\Epubviewer\Db\BookmarkMapper"
}
]
},
{
"file": "/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php",
"line": 124,
"function": "resolve",
"class": "OC\AppFramework\Utility\SimpleContainer",
"type": "->",
"args": [
"OCA\Epubviewer\Db\BookmarkMapper"
]
},
{
"file": "/var/www/html/lib/private/AppFramework/DependencyInjection/DIContainer.php",
"line": 451,
"function": "query",
"class": "OC\AppFramework\Utility\SimpleContainer",
"type": "->",
"args": [
"OCA\Epubviewer\Db\BookmarkMapper"
]
},
{
"file": "/var/www/html/lib/private/AppFramework/DependencyInjection/DIContainer.php",
"line": 423,
"function": "queryNoFallback",
"class": "OC\AppFramework\DependencyInjection\DIContainer",
"type": "->",
"args": [
"OCA\Epubviewer\Db\BookmarkMapper"
]
},
{
"file": "/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php",
"line": 74,
"function": "query",
"class": "OC\AppFramework\DependencyInjection\DIContainer",
"type": "->",
"args": [
"OCA\Epubviewer\Db\BookmarkMapper",
true
]
},
{
"function": "OC\AppFramework\Utility\{closure}",
"class": "OC\AppFramework\Utility\SimpleContainer",
"type": "->",
"args": [
"*** sensitive parameters replaced ***"
]
},
{
"file": "/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php",
"line": 98,
"function": "array_map",
"args": [
{
"__class__": "Closure"
},
[
"*** sensitive parameters replaced ***"
]
]
},
{
"file": "/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php",
"line": 106,
"function": "buildClass",
"class": "OC\AppFramework\Utility\SimpleContainer",
"type": "->",
"args": [
{
"__class__": "ReflectionClass",
"name": "OCA\Epubviewer\Service\BookmarkService"
}
]
},
{
"file": "/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php",
"line": 124,
"function": "resolve",
"class": "OC\AppFramework\Utility\SimpleContainer",
"type": "->",
"args": [
"OCA\Epubviewer\Service\BookmarkService"
]
},
{
"file": "/var/www/html/lib/private/AppFramework/DependencyInjection/DIContainer.php",
"line": 451,
"function": "query",
"class": "OC\AppFramework\Utility\SimpleContainer",
"type": "->",
"args": [
"OCA\Epubviewer\Service\BookmarkService"
]
},
{
"file": "/var/www/html/lib/private/AppFramework/DependencyInjection/DIContainer.php",
"line": 423,
"function": "queryNoFallback",
"class": "OC\AppFramework\DependencyInjection\DIContainer",
"type": "->",
"args": [
"OCA\Epubviewer\Service\BookmarkService"
]
},
{
"file": "/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php",
"line": 74,
"function": "query",
"class": "OC\AppFramework\DependencyInjection\DIContainer",
"type": "->",
"args": [
"OCA\Epubviewer\Service\BookmarkService",
true
]
},
{
"function": "OC\AppFramework\Utility\{closure}",
"class": "OC\AppFramework\Utility\SimpleContainer",
"type": "->",
"args": [
"*** sensitive parameters replaced ***"
]
},
{
"file": "/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php",
"line": 98,
"function": "array_map",
"args": [
{
"__class__": "Closure"
},
[
{
"__class__": "ReflectionParameter",
"name": "appName"
},
{
"__class__": "ReflectionParameter",
"name": "request"
},
{
"__class__": "ReflectionParameter",
"name": "urlGenerator"
},
{
"__class__": "ReflectionParameter",
"name": "rootFolder"
},
{
"__class__": "ReflectionParameter",
"name": "shareManager"
},
"And 3 more entries, set log level to debug to see all entries"
]
]
},
{
"file": "/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php",
"line": 106,
"function": "buildClass",
"class": "OC\AppFramework\Utility\SimpleContainer",
"type": "->",
"args": [
{
"__class__": "ReflectionClass",
"name": "OCA\Epubviewer\Controller\PageController"
}
]
},
{
"file": "/var/www/html/lib/private/AppFramework/Utility/SimpleContainer.php",
"line": 124,
"function": "resolve",
"class": "OC\AppFramework\Utility\SimpleContainer",
"type": "->",
"args": [
"OCA\Epubviewer\Controller\PageController"
]
},
{
"file": "/var/www/html/lib/private/AppFramework/DependencyInjection/DIContainer.php",
"line": 451,
"function": "query",
"class": "OC\AppFramework\Utility\SimpleContainer",
"type": "->",
"args": [
"OCA\Epubviewer\Controller\PageController"
]
},
{
"file": "/var/www/html/lib/private/AppFramework/DependencyInjection/DIContainer.php",
"line": 423,
"function": "queryNoFallback",
"class": "OC\AppFramework\DependencyInjection\DIContainer",
"type": "->",
"args": [
"OCA\Epubviewer\Controller\PageController"
]
},
{
"file": "/var/www/html/lib/private/AppFramework/App.php",
"line": 140,
"function": "query",
"class": "OC\AppFramework\DependencyInjection\DIContainer",
"type": "->",
"args": [
"OCA\Epubviewer\Controller\PageController"
]
},
{
"file": "/var/www/html/lib/private/Route/Router.php",
"line": 303,
"function": "main",
"class": "OC\AppFramework\App",
"type": "::",
"args": [
"OCA\Epubviewer\Controller\PageController",
"showReader",
{
"__class__": "OC\AppFramework\DependencyInjection\DIContainer"
},
{
"_route": "epubviewer.page.showreader"
}
]
},
{
"file": "/var/www/html/lib/base.php",
"line": 1003,
"function": "match",
"class": "OC\Route\Router",
"type": "->",
"args": [
"/apps/epubviewer/"
]
},
{
"file": "/var/www/html/index.php",
"line": 24,
"function": "handleRequest",
"class": "OC",
"type": "::",
"args": []
}
],
"File": "/var/www/html/apps-extra/epubviewer/lib/Db/BookmarkMapper.php",
"Line": 23,
"message": "OCA\Epubviewer\Db\BookmarkMapper::__construct(): Argument #2 ($userId) must be of type string, null given",
"exception": {},
"CustomMessage": "OCA\Epubviewer\Db\BookmarkMapper::__construct(): Argument #2 ($userId) must be of type string, null given"
}
}

In the meantime I’m using an ugly workaround which consists of building mocks of these dependencies if they can’t be built and feed them to my Controller…
https://github.com/devnoname120/epubviewer/commit/61279ea78f0105fa53dd66ccf0af693ed0e84d9f

Could you help me fix it? Thanks!

2

Just to avoid the dumb part: You are using a NC installation that has the support already included, right?

Apart from that, you could start debugging the (core) core.

Chris

3

I’m not entirely sure what you mean but if this is your question this feature was introduced in Nextcloud 28 and I’m using Nextcloud 30.0.6.

How can I debug the “core (core)” I have almost zero knowledge about the Nextcloud codebase or the intricacies of dependency injection

4

Long story shourt, you would need a dev environment (e.g. this one is used for many projects).

Then, you can enable step debugging and dig into the core and see all the glory details of the dependency injection system implemented. However, this is no easy undertaking. I just warn you to not expect results in mere minutes if you do not have much experience there (and need to set up the env first).

And yes, this was what I meant to make sure you are on a recent enough version of the core.

5

@christianlupus Unfortunately it’s too much work for me considering I don’t know the codebase. It’s just not worth learning about all of this, how the dependency injector is designed, its trade-offs, how to debug, and so on.

Fortunately I was able to work around this error in a much more elegant way, without the complicated mocks.

The solution is to mark the injection service as nullable: Get rid of the horrible Null Mappers workaround · devnoname120/epubviewer@19e6b8e · GitHub

And then register the service with a custom factory function which simply returns null if it can’t be built: Get rid of the horrible Null Mappers workaround · devnoname120/epubviewer@19e6b8e · GitHub

Hopefully this will help the next person who has that issue!

My hunch is that optional dependency injections only works with services, not when injecting dependencies to controllers. Haven’t tested or checked in the code so I may be wrong it’s possible that the entire thing is broken.

6

Hey again,

I thought it would be feasible to give it a shot once. But I fail to get tho the problem. Maybe I am simply not going the right path. Here is what I did:

  1. Clone the git repo to my dev environment
  2. Install NPM packages and build the app (using make npm)
  3. Tried to install composer packages and failed for missing autoload configuration (here you seem to have another bug, maybe this also causes the problem…)
  4. Enabled the app in the NC instance
  5. Uploaded a random EPUB to the instance’s files and clicked on it => No error as I am logged in
  6. Shared publicly as link and opend in private browser window
  7. Download is possible but no epub is shown/rendered

The only way to trigger an error was when I open the EPUB as logged in user and use the very same link in a private window. Is that the intended use-case?

7

I dug a bit into the DI code of the server and found also the direct cause of the problem that I described above:

You did successfully define the userId parameter as nullable in the controller. However, you also request objects of BookmarkService and PreferenceService. Going down the route further, the BookmarkService (although nullable, the server tries to build it) is needed. To build it, a BookmarkMapper is needed. And the BookmarkMapper has a hard dependency on the non-nullable userId. Similarly, the PreferenceMapper has a similar issue.

As a result, not the controller causes the error but your internal dependencies. I made the userIds nullable and tried again. This failed later on somewhere in the code somewhere in the controller method. So, this allowed the DI container to successfully build the class objects and start executing it.

I hope this explains the problem for you.

Chris

8

Thank you for your tests. I’m a bit confused about the usefulness of optional dependencies if these break if one of their dependencies cannot be built.

My expectation when I use an optional dependency in a controller is that it would simply make it null if it fails to build/inject it (no matter the reason, could be because it fails in the constructor, or because any of its dependencies fails to be fetched/built, or something else). IMO the fact that the optionally injected dependency has a transitive dependency that cannot be satisfied (e.g. here $userId) shouldn’t make any difference in the behavior.

I’m asking to optionally inject a service, if it can’t then it should just make it null rather than bubbling up an exception that prevents my controller from handling the request at all. That exception should be caught by default by the Nextcloud injection services without requiring me to add a custom injection logic with some manual try { … } catch { return null; }

If I want to do more fine-grained error-handling then I’m free to implement a custom injector, but the default should be that I don’t need to. Otherwise from my PoV optional dependencies are entirely useless and I would rather not have them at all because they have unexpected failure modes where it’s unclear if I’ll receive a null or if the whole thing will break because somewhere some transitive dependency depends on something that can’t be injected…

On another note for your tests you should use make dist instead (sorry it’s not clear in the docs…)

For context:

I haven’t updated the documentation yet but for development (read: linting) I’m using a workaround which requires to run git clone --branch stable31 --filter=blob:none --recurse-submodules --also-filter-submodules https://github.com/nextcloud/server nextcloud-server from the epubviewer folder because I have a custom autoload-dev that defines some PSR-4 namespaces that point directly to the Nextcloud server to get proper up-to-date typings on everything.

Reason is that I don’t want to rely on @nextcloud/ocp because I’ve had bad surprises with it in the past + I have some legacy stuff such as \OC that hasn’t yet been migrated.

So whenever you need to test you have to redump the autoload without dev.

I think this should work (untested because I usually just run make dist instead):

COMPOSER_NO_DEV=1 php $(build_tools_directory)/composer.phar install --prefer-dist && php $(build_tools_directory)/composer.phar dump-autoload --no-dev
9

OK, I first thought the use case is different than yours: I thought this is intended to allow for inter-app communication. Then, you could just inject the other app in question. If it was not installed, you would get null values.

I reconsidered and looked at the official docs that I consider the API/contract to stick to. There, it is written

If an injected dependency can’t be found or build, an exception is thrown. This can be avoided by using the a nullable type notation for a dependency

So, I consider this a bug of the DI core. I want to create a minimal working example and would verify there that all assumptions are correct. After that: You want to file the bug or should I?

10

I opened the issue [Bug]: Nullable dependencies might trigger 500 return value · Issue #51573 · nextcloud/server · GitHub on GitHub.

1 Like
11

Perfect thank you a lot!

1 Like