Skip to content

Conversation

@alyssasuiyiguo
Copy link
Contributor

No description provided.

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.

Summary of Changes

Hello @alyssasuiyiguo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This PR introduces a new C++ client library designed to interact with the Data Commons API. It provides structured methods for fetching data, specifically implementing functionalities to retrieve property values and observations from the API. The library leverages modern C++ features and integrates popular third-party libraries for HTTP requests and JSON parsing, making it a robust foundation for C++ applications needing Data Commons integration.

Highlights

  • New C++ Client Library: A foundational C++ client library for the Data Commons API has been added, enabling C++ applications to programmatically access Data Commons data.
  • API Interaction: Implemented GetPropertyValues and GetObservations methods to query the Data Commons API for specific data points, handling request construction and response parsing.
  • Dependency Management: Utilizes CMake's FetchContent to manage external dependencies like cpr for HTTP requests and nlohmann/json for efficient JSON serialization and deserialization.
  • Build System Setup: A comprehensive CMakeLists.txt is included to facilitate easy building of the library and an example executable, ensuring cross-platform compatibility.
  • Example Usage: An examples/main.cpp file demonstrates how to initialize the client and make calls to the GetObservations (and GetPropertyValues as a commented-out example) endpoint, providing a quick start for users.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

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 a new C++ client library for the Data Commons API. The implementation uses cpr for HTTP requests and nlohmann/json for JSON handling, which are fetched via CMake's FetchContent. The overall structure is good, but there are a few important issues to address. Most critically, a hardcoded API key is present in the example code, which is a major security risk. There is also a bug in GetPropertyValues where a property name is hardcoded. Additionally, I've provided some suggestions to improve code clarity and error handling.

Comment on lines 5 to 6
// NOTE: Please provide a valid API key.
datacommons::DataCommons dc("AIzaSyCTI4Xz-UW_G2Q2RfknhcfdAnTHq5X5XuI");
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

A hardcoded API key is present in the example. This is a major security vulnerability. API keys and other secrets should never be committed to source code. It's recommended to load the key from an environment variable or a configuration file at runtime.

To use std::getenv, you'll also need to add #include <cstdlib> at the top of the file.

Suggested change
// NOTE: Please provide a valid API key.
datacommons::DataCommons dc("AIzaSyCTI4Xz-UW_G2Q2RfknhcfdAnTHq5X5XuI");
// NOTE: Please provide a valid API key via the DATACOMMONS_API_KEY environment variable.
const char* api_key = std::getenv("DATACOMMONS_API_KEY");
if (!api_key) {
std::cerr << "Error: DATACOMMONS_API_KEY environment variable not set." << std::endl;
return 1;
}
datacommons::DataCommons dc(api_key);

Comment on lines 31 to 33
std::string arc_prop = "name";
if (data["arcs"].contains(arc_prop)) {
for (const auto& node : data["arcs"][arc_prop]["nodes"]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The property to look for in the response JSON is hardcoded to "name". This is incorrect and should use the prop argument passed to the function. This bug prevents retrieving values for any property other than name.

                if (data["arcs"].contains(prop)) {
                    for (const auto& node : data["arcs"][prop]["nodes"]) {

namespace datacommons {

struct PropertyValue {
std::string dcid;
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The field dcid in the PropertyValue struct is misleading. The implementation populates this field with the provenanceId from the API response, not a dcid. Renaming it to provenance_id would make the code clearer and less prone to misuse.

Suggested change
std::string dcid;
std::string provenance_id;

Comment on lines 108 to 109
std::cerr << "Error: " << r.status_code << " - " << r.error.message << std::endl;
std::cerr << r.text << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Writing errors directly to std::cerr is not ideal for a library, as it removes control from the client application on how to handle or log errors. A better approach would be to propagate errors to the caller, for example by throwing an exception or returning an object that encapsulates the result or error. This would allow the user of your library to implement their own error handling logic (e.g., logging to a file, showing a UI message).

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