Skip to content

[Firebase AI] Add support for JsonSchema#1419

Open
a-maurice wants to merge 1 commit intomainfrom
am-json_schema
Open

[Firebase AI] Add support for JsonSchema#1419
a-maurice wants to merge 1 commit intomainfrom
am-json_schema

Conversation

@a-maurice
Copy link
Collaborator

Description

Provide details of the change, and generalize the change in the PR title above.

Add the JsonSchema class, which is mostly a duplicate of the Schema object, which follows the OpenAPI schema. They support refs and defs, for Object definition.


Testing

Describe how you've tested these changes.

Running tests locally.


Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for JsonSchema formatting in the Firebase AI library by adding a new JsonSchema class and integrating it into FunctionDeclaration and GenerationConfig for structured data definitions. The changes also include a new automated test case and minor cleanups in the existing Schema class. Review feedback points out a syntax error in FunctionCalling.cs involving a double semicolon and suggests refactoring the JsonSchema and Schema classes to use a common base class to reduce code duplication and improve maintainability.

Name = name;
Description = description;
Parameters = null;
JsonParameters = JsonSchema.Object(parameters, optionalParameters);;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

There's a syntax error here due to a double semicolon ;;. This will cause a compilation failure.

      JsonParameters = JsonSchema.Object(parameters, optionalParameters);

/// These types can be objects, but also primitives and arrays. Represents a select subset of an
/// [JsonSchema object](https://json-schema.org/specification).
/// </summary>
public class JsonSchema
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This new JsonSchema class is almost a complete duplicate of the existing Schema class. This introduces a significant amount of code duplication, which will make future maintenance harder, as bug fixes and enhancements will need to be applied in two places.

Consider refactoring to reduce duplication. One approach would be to create a generic base class (e.g., SchemaBase<T>) that contains the common logic, and then have both Schema and JsonSchema inherit from it.

For example:

public abstract class SchemaBase<T> where T : SchemaBase<T>
{
    // ... common properties like Type, Description, etc.
    // Properties that refer to the schema type would use T
    public T Items { get; }
    public IReadOnlyDictionary<string, T> Properties { get; }
    // ...

    protected SchemaBase(...) { /* ... */ }
}

public class JsonSchema : SchemaBase<JsonSchema>
{
    // ... JsonSchema specific logic and factory methods
}

public class Schema : SchemaBase<Schema>
{
    // ... Schema specific logic and factory methods
}

This would greatly improve the maintainability of the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant