-
Notifications
You must be signed in to change notification settings - Fork 690
Fix INSERT INTO SELECT FROM parameter overwriting data with defaults #2210
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
Conversation
Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
…rder Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
mathiasrw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
mathiasrw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
I am seeing this in my CICD output. Can you try to undestand what is happening. Also please make sure no mocha test is echoing during test execution. |
|
@copilot did you see the comment? |
Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
Removed Note: The other console output visible in the CICD (like "res1 (SELECT *):", "Error message: Foreign key...") comes from other test files in the repository, not from the tests I added. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot From CICD |
Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
... Fixed the regression in test400. The issue was that my initial fix was overwriting all properties with defaults, including undefined ones. Changed the logic to only apply defaults for properties that don't exist in the source record using All tests now pass including test400 and test1484. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot I get nervous when our test case for this functionality did not pick this up. I think we need to broaden our test cases to cover more edge where we might have affected the insert functionality. |
Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
Added 5 additional edge case tests to test1484.js to provide comprehensive coverage of INSERT with defaults scenarios. The test suite now includes 7 tests covering:
These tests would have caught the test400 regression. All 519 tests pass. Commit: 553f5ec Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Description
When inserting data via
INSERT INTO table SELECT * FROM ?with parameterized data arrays, column values were being replaced with empty defaults instead of preserving source data.Root cause: In
src/70insert.js:246, defaults were being applied incorrectly usingextend(), which either:undefinedvalues from source records (regression in initial fix)Fix: Changed the default application logic to only set default values for properties that don't exist in the source record. The new logic:
table.defaultfnskey in rcheckundefinedvaluesExample of the fixed behavior:
This also correctly handles triggers with DEFAULT values:
Changes
for(var key in defaults){if(!(key in r)){r[key]=defaults[key]}}These comprehensive tests ensure INSERT with defaults functionality works correctly across all edge cases and would have caught the test400 regression during initial development.
Checklist
Warning
<issue_title>Records in table are always insert empty in the table</issue_title>
<issue_description>Hi We are planing to upgrade the alssql version from .2 to 1.7.3. We are using the browser version of alasql and download from https://cdn.jsdelivr.net/npm/alasql@1.7( Got this link from official site (https://github.com/AlaSQL/alasql) and include in the our webpage. But with latest records in table are always insert empty in the table.
We are trying to create the table 'BCPartners' and then insert the data into JSON formate. We can see the records set but each column has blank value.This is a blocker for us so need help ASAP
Below are the sample code and more details
-----------
Sample HTML page
---------------------
---------- Sample JS (test.js) code ---------
</issue_description>
<agent_instructions>Can you please have a look at this?</agent_instructions>
Comments on the Issue (you are @copilot in this section)
@mathiasrw @harish343 Absolutely @mathiasrw @harish343 Use `https://cdn.jsdelivr.net/npm/alasql@2.1.4/dist/alasql.js` instead so you dont get the minified version.@amiagarw what is in the test.js file?</comment_new>
<comment_new>@mathiasrw
alasql is the database</comment_new>
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.