Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unknown service nitric.proto.sql.v1.Sql error during collection #812

Open
tjholm opened this issue Oct 30, 2024 · 2 comments
Open

Unknown service nitric.proto.sql.v1.Sql error during collection #812

tjholm opened this issue Oct 30, 2024 · 2 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@tjholm
Copy link
Member

tjholm commented Oct 30, 2024

Raising as an issue from:
https://discord.com/channels/955259353043173427/955259353043173430/1300940268618383421

Example app: https://github.com/chimon2000/notes_nitric

The issue occurring in this example is that the Sql runtime API like our other runtime APIs is not registered during collection time, this is intended by design, this should be linked to produce an error during collection that highlights the issue though so this is definitely a bug.

However reading a connection string isn't really a side-effect producing operation, so it is possible to make an exception in this case, but doing so will require documentation of specific exceptions to this rule.

The example provided above is written in Dart, and being able to support this style of retrieval of the connection string in this specific case is very convenient given all global variables in Dart are lazily evaluated.

There are a few options to address this:

Option 1 - Implement the connection string SQL method during requirements gathering

Add the SQL runtime server during requirements gathering and have it return an empty string or dummy value.

The main issue with this solution is that runtime errors that occur due to code that contains side-effects will be linked to having an incorrect connection string rather than there being no database to reference at collection time.

Option 2 - Allow application level nitric lifecycle execution

This is already possible using env variables, but implementing this at the SDK level would make this more convenient.

A working example without SDK enhancement:

if (process.env.NITRIC_ENVIRONMENT == "build") {
   // Code to execute at build time
}

if (process.env.NITRIC_ENVIRONMENT == "run") {
  // Code the execute during local simulation
}

if (process.env.NITRIC_ENVIRONMENT == "cloud") {
  // Code the execute during deployed runs
}

In the specific example linked above using the getConnection() method as an example:

import 'package:nitric_sdk/nitric.dart';
import 'package:nitric_sdk/src/api/sql.dart';
import 'package:postgres/postgres.dart';

SqlDatabase get db => Nitric.sql(
      'notes',
      migrations: "file://db/migrations/notes",
    );

Future<Connection?> getConnection() async {
  // We need to ensure that db is reference regardless of the execution phase so it is evaluated
  final dbRef = db;
  final nitricEnv = Platform.environment['NITRIC_ENVIRONMENT'];
  
  if (nitricEnv == "build") {
    print('Skipping connection during nitric build');
    return null;
  }
  
  final uri = Uri.parse(await db.connectionString());

  final userInfoParts = uri.userInfo.split(':');
  final username = userInfoParts.length == 2 ? userInfoParts[0] : null;
  final password = userInfoParts.length == 2 ? userInfoParts[1] : null;
  final isUnixSocketParam = uri.queryParameters['is-unix-socket'];
  final applicationNameParam = uri.queryParameters['application_name'];
  final endpoint = Endpoint(
    host: uri.host,
    port: uri.port,
    database: uri.path.substring(1),
    username: username ?? uri.queryParameters['username'],
    password: password ?? uri.queryParameters['password'],
    isUnixSocket: isUnixSocketParam == '1',
  );

  final settings = ConnectionSettings(
    applicationName: applicationNameParam,
    sslMode: SslMode.values.byName(uri.queryParameters['sslmode'] ?? 'disable'),
  );

  return Connection.open(endpoint, settings: settings);
}

While more verbose that Option 1, the advantage is that the reason for this behavior is documented with the application code.

@tjholm tjholm added bug Something isn't working good first issue Good for newcomers labels Oct 30, 2024
@chimon2000
Copy link

Option 2 seems to be the better option since this can be abstracted into the current SDK implementation and it seems resilient to the side effects identified in option 1

@tjholm
Copy link
Member Author

tjholm commented Nov 5, 2024

Just putting a rough proposal for an SDK level abstraction of the above code but using typescript as an example:

import * as nitric from "@nitric/sdk";

nitric.atRuntime(() => {
  console.log("runtime");
});

nitric.atBuildTime(() => {
  console.log("build");
});

@tjholm tjholm self-assigned this Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants