Add support for RESTful APIs as a data source #34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request introduces the ability to connect to RESTful APIs, refactors the data upload logic to handle both CSV and API data, updates the logo in the drawer header, and updates the data processing logic for CSV files to handle the data correctly. It also removes the No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
sourcery-ai
bot
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cbrianbet - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting the API request logic into a separate reusable function to reduce duplication in
handleClickandhandleConnectionTest. - The
bodyproperty is commented out in the API request, make sure this is intentional and add a comment explaining why.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| setAlertMessage('Data does not contain JSON key in result') | ||
| } | ||
| } | ||
| setTestLoader(false) |
There was a problem hiding this comment.
suggestion: Streamline testLoader reset using a finally block.
In both API and non-API branches, setTestLoader(false) is explicitly called, and an extra call exists after the conditional block. Using a finally block would guarantee that testLoader is reset without repetitive calls.
Suggested implementation:
}
} catch (error) {
setAlertType('error')
setAlertMessage(`API connection failed! ${responseData}`)
} finally {
setTestLoader(false)Ensure that any other explicit calls to setTestLoader(false) (if present further down or in other branches of the try block) are removed. Additionally, verify that the API error handling logic (including calls to setAlertMessage) is consistent after this refactoring.
| } catch (error) { | ||
| setAlertType('error') | ||
| console.error('Error testing API connection:', JSON.stringify(error)); | ||
| setAlertMessage(error) |
There was a problem hiding this comment.
suggestion: Consider improving error message handling in API fetch.
The current approach sets the alert message directly with the error object. Extracting a more descriptive message (e.g., using error.message) could provide clearer feedback to the user.
Suggested change
| setAlertMessage(error) | |
| setAlertMessage(error?.message || 'An unknown error occurred while testing the API connection.') |
| let conn_str = `${formData.db_type}://${formData.username}:${formData.password}@${formData.host_port}/${formData.database}` | ||
| onNextStep({conn_str, conn_type: formData.conn_type}); | ||
| } else if(handleValidationAPI() && formData.db_type === 'api'){ |
There was a problem hiding this comment.
issue (complexity): Consider extracting the API connection logic into a reusable utility function to reduce code duplication and deep nesting in handleClick and handleConnectionTest functions, improving maintainability and readability .
Consider extracting the common API connection logic into a small utility so that both handleClick and handleConnectionTest reuse the same logic. This reduces duplication and deep nesting.
For example, you can create a helper function:
const executeAPIRequest = async (apiData, onSuccess, onError) => {
try {
const response = await fetch(apiData.endpoint, {
method: apiData.method,
headers: { "Content-Type": "application/json" },
// body: JSON.stringify(apiData.body), // uncomment when needed
});
const responseData = await response.json();
if (!response.ok) {
throw new Error(`API connection failed! ${JSON.stringify(responseData)}`);
}
if (apiData.dataPath && !responseData.hasOwnProperty(apiData.dataPath)) {
throw new Error("Data does not contain JSON key in result");
}
onSuccess(apiData.dataPath ? responseData[apiData.dataPath] : responseData);
} catch (error) {
onError(error);
}
};Then update your handlers accordingly. For instance, in handleClick:
const handleClick = async () => {
if (handleValidation() && formData.db_type !== "api") {
let conn_str = `${formData.db_type}://${formData.username}:${formData.password}@${formData.host_port}/${formData.database}`;
onNextStep({ conn_str, conn_type: formData.conn_type });
} else if (handleValidationAPI() && formData.db_type === "api") {
setTestLoader(true);
await executeAPIRequest(
apiFormData,
(data) => {
onNextStep({
conn_str: apiFormData.endpoint,
conn_type: formData.conn_type,
db: formData.db_type,
data,
});
setAlertType(null);
setTestLoader(false);
},
(error) => {
setAlertType("error");
setAlertMessage(error.message);
setTestLoader(false);
}
);
}
};Apply a similar refactoring in handleConnectionTest. This approach removes duplicated fetch logic, reduces deep nesting, and centralizes API error handling without reverting any functionality.
Comment on lines
+118
to
+133
| if (!response.ok) { | ||
| setAlertType('error') | ||
| setAlertMessage(`API connection failed! ${responseData}`) | ||
| } else { | ||
| if (responseData.hasOwnProperty(apiFormData.dataPath) || apiFormData.dataPath === '') { | ||
| onNextStep({ | ||
| conn_str: apiFormData.endpoint, | ||
| conn_type: formData.conn_type, | ||
| db: formData.db_type, | ||
| data: responseData[apiFormData.dataPath] ?? responseData | ||
| }) | ||
| } else { | ||
| setAlertType('error') | ||
| setAlertMessage('Data does not contain JSON key in result') | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): Merge else clause's nested if statement into else if (merge-else-if)
Suggested change
| if (!response.ok) { | |
| setAlertType('error') | |
| setAlertMessage(`API connection failed! ${responseData}`) | |
| } else { | |
| if (responseData.hasOwnProperty(apiFormData.dataPath) || apiFormData.dataPath === '') { | |
| onNextStep({ | |
| conn_str: apiFormData.endpoint, | |
| conn_type: formData.conn_type, | |
| db: formData.db_type, | |
| data: responseData[apiFormData.dataPath] ?? responseData | |
| }) | |
| } else { | |
| setAlertType('error') | |
| setAlertMessage('Data does not contain JSON key in result') | |
| } | |
| } | |
| if (!response.ok) { | |
| setAlertType('error') | |
| setAlertMessage(`API connection failed! ${responseData}`) | |
| } | |
| else if (responseData.hasOwnProperty(apiFormData.dataPath) || apiFormData.dataPath === '') { | |
| onNextStep({ | |
| conn_str: apiFormData.endpoint, | |
| conn_type: formData.conn_type, | |
| db: formData.db_type, | |
| data: responseData[apiFormData.dataPath] ?? responseData | |
| }) | |
| } | |
| else { | |
| setAlertType('error') | |
| setAlertMessage('Data does not contain JSON key in result') | |
| } | |
Explanation
Flattening if statements nested within else clauses generates code that iseasier to read and expand upon.
Comment on lines
+208
to
+219
| if (!response.ok) { | ||
| setAlertType('error') | ||
| setAlertMessage(`API connection failed! ${responseData}`) | ||
| } else { | ||
| if (responseData.hasOwnProperty(apiFormData.dataPath) || apiFormData.dataPath === '') { | ||
| setAlertType('success') | ||
| setAlertMessage('Successfully connected to API') | ||
| } else { | ||
| setAlertType('error') | ||
| setAlertMessage('Data does not contain JSON key in result') | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): Merge else clause's nested if statement into else if (merge-else-if)
Suggested change
| if (!response.ok) { | |
| setAlertType('error') | |
| setAlertMessage(`API connection failed! ${responseData}`) | |
| } else { | |
| if (responseData.hasOwnProperty(apiFormData.dataPath) || apiFormData.dataPath === '') { | |
| setAlertType('success') | |
| setAlertMessage('Successfully connected to API') | |
| } else { | |
| setAlertType('error') | |
| setAlertMessage('Data does not contain JSON key in result') | |
| } | |
| } | |
| if (!response.ok) { | |
| setAlertType('error') | |
| setAlertMessage(`API connection failed! ${responseData}`) | |
| } | |
| else if (responseData.hasOwnProperty(apiFormData.dataPath) || apiFormData.dataPath === '') { | |
| setAlertType('success') | |
| setAlertMessage('Successfully connected to API') | |
| } | |
| else { | |
| setAlertType('error') | |
| setAlertMessage('Data does not contain JSON key in result') | |
| } | |
Explanation
Flattening if statements nested within else clauses generates code that iseasier to read and expand upon.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
New Features: