A better idea to set these settings would be using services.navidrome.settings itself.
I'm not sure but I think that it would also give an error in case a user tries to change settings.DataFolder directly, because the option is already defined in this module (it still can be done but requires lib.mkForce).

I don't think to understand what you mean, could you provide an example on how you intend to edit the code?
I tried to explicitly set settings.DataFolder but it is correctly overwritten, moreover an user reading the documentation has no idea that such an option exists.

could you provide an example on how you intend to edit the code?

Sure, I meant something like this:

{
  config = mkIf cfg.enable {

    environment.etc."navidrome/settings.json".text = 
      builtins.toJSON cfg.settings;

    services.navidrome.settings = {
      DataFolder = dataFolder;
      ConfigFile = configFile;
    } // optionalAttrs (hasAttrByPath ["Port"] cfg.settings) {
      Port = builtins.toString cfg.settings.Port;
    };

  };

}

moreover an user reading the documentation has no idea that such an option exists.

Well, to change navidrome.settings they are supposed to read the documentation you linked anyway, no?

Well, to change navidrome.settings they are supposed to read the documentation you linked anyway, no?

Oh. yes, you're right, I thought you meant the NixOS docs, not the navidrome one.

optionalAttrs (hasAttrByPath ["Port"] cfg.settings) {
      Port = builtins.toString cfg.settings.Port;
}

I cannot build this:

error: infinite recursion encountered, at /path/to/.../nixpkgs/lib/modules.nix:383:9
(use '--show-trace' to show detailed location information)

Any suggestion?

Oh right, that was pretty stupid: it's recursive. You could use the apply argument of mkOption to modify the settings after the evaluation but it seems it's not necessary to convert it to a string. I made a quick test with

import ./make-test-python.nix ({ pkgs, ... }: {
  name = "navidrome";

  machine = { ... }: {
    services.navidrome.enable = true;
    services.navidrome.settings = {
      MusicFolder = "/var/music";
      Port = 4535;
    };
  };

  testScript = ''
    machine.wait_for_unit("navidrome")
    machine.wait_for_open_port("4535")
  '';
})

and it works. You could add this to nixos/tests. Even if it's trivial is good to chek if the nixos module evaluates and there are no errors in the navidrome config file.

So I kept the module derivation as it was and I added your test. Is this ok?

You should really drop the part about converting settings.Port: navidrome can handle numeric values just fine, there's no need to convert it to a string.

Also, a nice thing would be to add the test to the list in nixos/tests/all-test.nix list and link it to the package expression. Try something like this

--- a/pkgs/servers/misc/navidrome/default.nix
+++ b/pkgs/servers/misc/navidrome/default.nix
@@ -1,4 +1,4 @@
-{ stdenv, fetchurl, ffmpeg, ffmpegSupport ? true, makeWrapper }:
+{ stdenv, fetchurl, ffmpeg, ffmpegSupport ? true, makeWrapper, nixosTests }:
 
 with stdenv.lib;
 
@@ -26,6 +26,8 @@ stdenv.mkDerivation rec {
     wrapProgram $out/bin/navidrome \
       --prefix PATH : ${makeBinPath (optional ffmpegSupport ffmpeg)}
   '';
+
+  passthru.tests.navidrome = nixosTests.navidrome;
   
   meta = {
     description = "Navidrome Music Server and Streamer compatible with Subsonic/Airsonic";

In this way @GrahamcOfBorg will run the test automatically next time you update the package.

We could potentially use the upstream systemd unit (https://github.com/deluan/navidrome/blob/master/contrib/navidrome.service) and override options as necessary by packaging it appropriately (moving to /lib/systemd/system directory) in the navidrome derivation.

It would be good to use the toml format from #75584

It would be nice to have the pkg configurable, to be able to use a version from unstable, for example.

Using an overlay accomplishes this nicely.

Good point!