fix: refuse bool and float item IDs everywhere instead of coercing them by minseokpark-CL · Pull Request #22 · CryptoLabInc/langchain-envector

minseokpark-CL · 2026-10-01T06:34:19Z

한국어

#21 위에 쌓은 PR 이다. base 는 feat/get-by-ids 이고, 이 화면에는 이 PR 의 변경만 보인다. #21 을 먼저 리뷰하고 merge 한 뒤 이 PR 의 base 를 main 으로 바꾼다.
CI(.github/workflows/pr.yml)는 base 가 main 인 PR 에서만 돌기 때문에, base 를 바꾸기 전까지 이 PR 에는 CI 결과가 없다. 아래 "검증" 은 로컬에서 돌린 결과다.

무엇이 문제였나

delete, update_metadata, update_documents, upsert_documents 와 add_texts / add_documents 의 ids 는 ID 를 int(...) 로 바로 바꿨다. int(True) 는 1, int(3.9) 와 int(3.0) 은 3 이다.
그래서 delete(ids=[3.9]) 는 item 3 을 지웠고, update_documents([3.9], ...) 와 add_texts(..., ids=[3.9]) 는 item 3 을 덮어썼다. 호출하는 쪽이 지정하지 않은 row 다.
feat: add get_by_ids, reading documents back by item ID #21 이전부터 있던 동작이다. feat: add get_by_ids, reading documents back by item ID #21 본문의 "알려진 문제" 절에서 이 PR 로 고친다고 적어 두었다.

무엇이 달라지나

모든 메서드가 _readable_item_id 하나로 ID 를 판별한다(feat: add get_by_ids, reading documents back by item ID #21 에서 get_by_ids 용으로 만든 것). item ID 는 양의 정수(numbers.Integral 이라 np.int64 같은 NumPy 정수 포함)와 그 ASCII 십진수 문자열뿐이고, bool·np.bool_ 과 float·np.float64 는 item ID 가 아니다. SDK 에는 Python int 로 넘긴다.
판별에 걸린 값의 처리는 메서드별 기존 방식을 따른다. delete / update_metadata / update_documents / upsert_documents 는 ValueError 를 내고 SDK 를 부르지 않는다. add_texts / add_documents 의 ids 는 새 row 로 넣고 UserWarning 을 낸다. get_by_ids 는 결과에서 뺀다.
0 과 음수는 지금처럼 "item IDs are positive integers" 메시지를 낸다.
"٣" 같은 ASCII 가 아닌 숫자와 2**63 이상은 item ID 로 보지 않는다. feat: add get_by_ids, reading documents back by item ID #21 의 get_by_ids 에 있던 같은 구멍은 feat: add get_by_ids, reading documents back by item ID #21 리뷰에서 함께 고쳐졌고(b642543), 이 PR 은 그 판별을 모든 메서드에 적용한다.
delete 와 add_texts 의 docstring 에 받는 형식을 적었다.

동작 비교:

넘긴 값	전: delete / update / upsert	후: delete / update / upsert	전: add_texts `ids`	후: add_texts `ids`
`True`	item 1	`ValueError`	item 1 덮어씀	새 row + `UserWarning`
`3.9`, `3.0`	item 3	`ValueError`	item 3 덮어씀	새 row + `UserWarning`
`"٣"`	item 3	`ValueError`	item 3 덮어씀	새 row + `UserWarning`
`3`, `"3"`, `" 3 "`, `np.int64(3)`	item 3	item 3	item 3	item 3

검증

python -m pytest tests -m "not integration" — ff04dc2 에서 146 passed. 새 unit test: delete 가 True·3.9·3.0·"٣" 에 ValueError 를 내고 SDK delete 를 부르지 않는 것, update_metadata·update_documents·upsert_documents 가 같은 값에 ValueError 를 내고 SDK 를 부르지 않는 것, add_texts 가 같은 값을 새 row 로 넣고 item 3 을 바꾸지 않는 것, int 와 십진수 문자열은 그대로 동작하는 것, 0 과 음수가 기존 메시지를 내는 것, get_by_ids 가 "٣" 를 빼는 것, NumPy 정수(int64·int32·uint64)는 item ID 로 읽고 np.bool_·np.float64 는 빼거나 ValueError 를 내는 것.
바뀐 것은 SDK 를 부르기 전의 ID 판별뿐이라 integration test 는 돌리지 않았다.

English

Stacked on #21: the base is feat/get-by-ids, so this view shows only this PR's change. Review and merge #21 first, then retarget this PR to main.
CI (.github/workflows/pr.yml) runs only on PRs whose base is main, so this PR shows no CI result until it is retargeted. "Verification" below was run locally.

What was wrong

delete, update_metadata, update_documents, upsert_documents and the ids of add_texts / add_documents turned each ID into int(...) directly. int(True) is 1; int(3.9) and int(3.0) are 3.
So delete(ids=[3.9]) deleted item 3, and update_documents([3.9], ...) and add_texts(..., ids=[3.9]) overwrote item 3 — rows the caller never named.
This predates feat: add get_by_ids, reading documents back by item ID #21. feat: add get_by_ids, reading documents back by item ID #21's "Known issue" section says this PR fixes it.

What changes

Every method checks IDs with one function, _readable_item_id (added in feat: add get_by_ids, reading documents back by item ID #21 for get_by_ids). An item ID is a positive integer (any numbers.Integral, so NumPy integers such as np.int64 count) or its ASCII decimal string only; bool / np.bool_ and float / np.float64 are not item IDs. The SDK receives a plain Python int.
What happens to a rejected value follows each method's existing rule. delete / update_metadata / update_documents / upsert_documents raise ValueError and do not call the SDK. The ids of add_texts / add_documents insert a new row and emit a UserWarning. get_by_ids leaves it out.
Zero and negatives keep their "item IDs are positive integers" message.
Non-ASCII digits such as "٣" and values of 2**63 or more are not item IDs either. The same hole in feat: add get_by_ids, reading documents back by item ID #21's get_by_ids was closed in feat: add get_by_ids, reading documents back by item ID #21's own review round (b642543); this PR applies that check to every method.
The delete and add_texts docstrings now state the accepted forms.

Behaviour, before and after:

Value passed	Before: delete / update / upsert	After: delete / update / upsert	Before: add_texts `ids`	After: add_texts `ids`
`True`	item 1	`ValueError`	overwrites item 1	new row + `UserWarning`
`3.9`, `3.0`	item 3	`ValueError`	overwrites item 3	new row + `UserWarning`
`"٣"`	item 3	`ValueError`	overwrites item 3	new row + `UserWarning`
`3`, `"3"`, `" 3 "`, `np.int64(3)`	item 3	item 3	item 3	item 3

Verification

python -m pytest tests -m "not integration" — 146 passed on ff04dc2. New unit tests: delete raises ValueError for True, 3.9, 3.0 and "٣" without calling the SDK delete; update_metadata, update_documents and upsert_documents raise ValueError for the same values without calling the SDK; add_texts inserts those as new rows and leaves item 3 untouched; ints and decimal strings still work; zero and negatives keep their message; get_by_ids leaves out "٣"; NumPy integers (int64, int32, uint64) are read as item IDs while np.bool_ and np.float64 are left out or raise ValueError.
Only the ID check that runs before the SDK call changed, so the integration tests were not run.

🤖 Generated with Claude Code

Kim-YeongHyeon

이 리뷰는 AI 에이전트가 전권 위임 하에 생성했습니다. 필수 부담당자 리뷰를 대체하지 않으며,
머지 판단은 사람 리뷰어에게 있습니다.

Kim-YeongHyeon · 2026-10-02T02:11:35Z

+    if isinstance(value, numbers.Integral):
+        return int(value) if value > 0 else None
+    if isinstance(value, str):
+        text = value.strip()
+        if text.isascii() and text.isdigit():


PR 본문의 표에 나온 값보다 더 많은 값이 거부됩니다. 아래 값은 35bfa33까지 int(v)로 item 3(또는 3000)을 가리켰지만, 이제 delete·update_*·upsert_documents에서는 ValueError가 나고 add_texts에서는 새 행으로 들어갑니다.

"+3", "3_000", 전각 "３"

b"3", Decimal(3), Fraction(3)

__index__만 정의한 객체(np.array(3), 정수 torch tensor 등)

의도한 범위라면 표와 release note에 breaking change로 적어 주십시오.

#23 9fc0667 에 반영했습니다. 거부 범위는 그대로 두고, 본문의 "무엇이 달라지나" 에 Breaking change 항목으로 적었습니다. 표에도 "+3"·"3_000"·전각·b"3"·Decimal·Fraction·np.array(3) 행을 넣었고, README Limitations 에 받는 형식(add_texts 가 돌려준 문자열, 또는 Python·NumPy 정수)을 적었습니다. 이 저장소에 release note 파일이 없어 PR 본문과 README 로 갈음합니다.

Kim-YeongHyeon · 2026-10-02T02:11:35Z

+        allows: an entry that is an item ID (a positive int or its decimal str,
+        such as the ``Document.id`` search results carry) updates that item in
+        place; an ID


add_texts의 ids로 3.0이나 np.float64(3.0)을 넘기면, 이전에는 item 3이 그 자리에서 갱신됐지만 이제는 새 행이 들어가고 UserWarning만 납니다. pandas 정수 열은 NaN이 하나만 있어도 float64가 되므로, 같은 데이터를 다시 적재할 때마다 행이 중복될 수 있습니다. 같은 값을 받은 delete·update_*·upsert_documents는 예외를 내서 바로 드러나는데, add_texts는 계속 씁니다.

"새 행 + 경고" 규칙은 UUID 같은 외부 ID를 위해 만든 것이니, 정수처럼 읽히지만 거부된 값(bool, float, 비ASCII 숫자)은 add_texts에서도 예외를 내거나 float.is_integer()를 받아 주는 것 가운데 하나로 정해 두면 좋겠습니다.

#23 9fc0667 에 반영했습니다. 규칙을 둘로 나눴습니다. 어떤 ID 도 될 수 없는 타입(float·np.float64 와 그 밖의 비정수 수, bool, bytes, np.array(3) 같은 __index__ 객체)은 add_texts 도 ValueError 를 냅니다 — 말씀하신 pandas float64 열 재적재 중복이 생기지 않습니다. item ID 가 아닌 문자열(UUID, "doc-7", 0 부터 시작하는 chunk 번호 "0", "+3", 비ASCII 숫자)은 지금처럼 새 row + UserWarning 입니다. LangChain 의 ID 타입이 문자열이라 문자열은 외부 ID 일 수 있고, "0" 을 새 row 로 넣는 기존 테스트(test_zero_is_not_an_item_id)가 그 의도를 이미 적어 두고 있어서입니다. float.is_integer() 를 받아 주는 쪽은 택하지 않았습니다. 3.9 를 거부하면서 3.0 만 받으면 규칙이 두 겹이 되고, 나중에 풀어 주는 것은 호환성을 깨지 않지만 반대는 깨기 때문입니다.

Kim-YeongHyeon · 2026-10-02T02:11:35Z

+def _is_non_positive_integer(value: Any) -> bool:
+    """True for an integer (or its ASCII decimal string, sign allowed) <= 0."""
+    if isinstance(value, bool):
+        return False
+    if isinstance(value, numbers.Integral):
+        return value <= 0
+    if isinstance(value, str):
+        text = value.strip()
+        digits = text[1:] if text[:1] in "+-" else text
+        return bool(digits) and digits.isascii() and digits.isdigit() and int(text) <= 0
+    return False


이 함수는 두 오류 메시지 가운데 하나를 고르려고 값을 한 번 더 파싱하는데, 부호 처리가 _readable_item_id와 다릅니다. "+0"은 "item IDs are positive integers" 메시지를 받고, "+3"은 "expects integer item IDs"를 받습니다. 부호가 있는 int나 None을 돌려주는 파서 하나로 합치고, 양수면 item ID, 0 이하면 "positive integers" 메시지를 내도록 하면 이 함수가 빠집니다.

#23 9fc0667 에 반영했습니다. _is_non_positive_integer 를 없애고 파서 하나(_parse_integer: bool 아닌 Integral, 또는 앞에 - 를 허용한 19자리 이하 ASCII 숫자 문자열)가 _readable_item_id 와 _mutation_items 양쪽을 먹입니다. 양수면 item ID, 0 이하면 "positive integers" 메시지, 그 밖은 "expects integer item IDs" 입니다. + 는 받지 않기로 해서 "+0" 과 "+3" 은 둘 다 expects-integer 메시지이고, "-0" 은 positive-integers 메시지입니다(test_signed_zero_and_plus_three_messages).

delete, update_metadata, update_documents and upsert_documents turned each id into int(...), and so did the ids of add_texts / add_documents. int(True) is 1 and int(3.9) is 3, so delete(ids=[3.9]) deleted item 3 and add_texts(..., ids=[3.9]) overwrote it — rows the caller never named. All of them now share _readable_item_id, the check get_by_ids uses: only a positive int or its ASCII decimal string is an item ID. What happens to anything else follows each method's existing rule — ValueError from delete / update_* / upsert_documents, a new row plus a UserWarning for the ids of add_texts. Non-positive ids keep their own "positive integers" message. The check also stops accepting non-ASCII digits ("٣" read item 3 through str.isdecimal), which get_by_ids did. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

The shared ID check accepted only Python int, so an ID taken from a NumPy array or a pandas column — np.int64(3) — was refused: delete([np.int64(3)]) raised ValueError and add_texts inserted a new row instead of updating item 3, where main had treated it as item 3. Check for numbers.Integral (minus bool) instead. NumPy integers are registered as Integral; np.bool_ and np.float64 are not, so they stay refused like bool and float. The ID passed on is a plain int. Also rewrap an add_texts docstring line that ran past the line length. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

minseokpark-CL · 2026-10-02T06:28:42Z

#22 는 2026-10-02 에 실수로 base(feat/get-by-ids)에 merge 되어, feat/get-by-ids 를 그 전 상태(b642543)로 되돌렸습니다. 같은 내용을 #23 으로 다시 열었고, 여기 남아 있는 리뷰 코멘트 3개는 #23 에서 답하겠습니다.

…ng-typed ids Review on #22 and #23 found four more ways an id could address a row the caller never named, or fail in the wrong place: - ids passed as a bare str or bytes iterated by element: delete("13") deleted items 1 and 3, delete(b"13") items 49 and 51. Every method that takes ids now refuses str / bytes / bytearray with a TypeError that says to pass a list, e.g. [doc.id]; tuples, sets and generators still work. The README delete example passed doc.id bare; it now passes [doc.id]. Pre-existing on main and in LangChain's own InMemoryVectorStore, but it is exactly what this PR exists to stop. - A digit string longer than 4300 characters made CPython's int() raise before any of our rules ran. A 19-digit cap (the length of 2**63 - 1) classifies such strings as "not an item ID" first. - add_texts inserted a new row with a warning for 3.0, True or np.float64(3.0). Those are wrongly-typed item IDs, not foreign IDs — a pandas integer column turns float64 on its first NaN — and a silent new row duplicates the document on every re-load. add_texts now raises for values that can be no ID at all (bool, float and other non-integral numbers, bytes, __index__ objects). Strings that are not item IDs ("0", "doc-7", a UUID, "+3", non-ASCII digits) still insert with a warning, since LangChain IDs are strings and 0-based chunk numbers are a common foreign ID. - _is_non_positive_integer parsed signs its own way, so "+0" and "+3" got different messages. One parser, _parse_integer, now feeds both _readable_item_id and _mutation_items: a leading "-" is read so 0 and negatives get the "positive integers" message, "+3" is simply not an item ID. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…em (#23) * fix: refuse bool and float item IDs everywhere instead of coercing them delete, update_metadata, update_documents and upsert_documents turned each id into int(...), and so did the ids of add_texts / add_documents. int(True) is 1 and int(3.9) is 3, so delete(ids=[3.9]) deleted item 3 and add_texts(..., ids=[3.9]) overwrote it — rows the caller never named. All of them now share _readable_item_id, the check get_by_ids uses: only a positive int or its ASCII decimal string is an item ID. What happens to anything else follows each method's existing rule — ValueError from delete / update_* / upsert_documents, a new row plus a UserWarning for the ids of add_texts. Non-positive ids keep their own "positive integers" message. The check also stops accepting non-ASCII digits ("٣" read item 3 through str.isdecimal), which get_by_ids did. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix: accept NumPy integers as item IDs The shared ID check accepted only Python int, so an ID taken from a NumPy array or a pandas column — np.int64(3) — was refused: delete([np.int64(3)]) raised ValueError and add_texts inserted a new row instead of updating item 3, where main had treated it as item 3. Check for numbers.Integral (minus bool) instead. NumPy integers are registered as Integral; np.bool_ and np.float64 are not, so they stay refused like bool and float. The ID passed on is a plain int. Also rewrap an add_texts docstring line that ran past the line length. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix: refuse a bare string as ids, cap digit strings, and raise on wrong-typed ids Review on #22 and #23 found four more ways an id could address a row the caller never named, or fail in the wrong place: - ids passed as a bare str or bytes iterated by element: delete("13") deleted items 1 and 3, delete(b"13") items 49 and 51. Every method that takes ids now refuses str / bytes / bytearray with a TypeError that says to pass a list, e.g. [doc.id]; tuples, sets and generators still work. The README delete example passed doc.id bare; it now passes [doc.id]. Pre-existing on main and in LangChain's own InMemoryVectorStore, but it is exactly what this PR exists to stop. - A digit string longer than 4300 characters made CPython's int() raise before any of our rules ran. A 19-digit cap (the length of 2**63 - 1) classifies such strings as "not an item ID" first. - add_texts inserted a new row with a warning for 3.0, True or np.float64(3.0). Those are wrongly-typed item IDs, not foreign IDs — a pandas integer column turns float64 on its first NaN — and a silent new row duplicates the document on every re-load. add_texts now raises for values that can be no ID at all (bool, float and other non-integral numbers, bytes, __index__ objects). Strings that are not item IDs ("0", "doc-7", a UUID, "+3", non-ASCII digits) still insert with a warning, since LangChain IDs are strings and 0-based chunk numbers are a common foreign ID. - _is_non_positive_integer parsed signs its own way, so "+0" and "+3" got different messages. One parser, _parse_integer, now feeds both _readable_item_id and _mutation_items: a leading "-" is read so 0 and negatives get the "positive integers" message, "+3" is simply not an item ID. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix: treat ids=None as "nothing to update" again, as on main 9fc0667 routed ids through _ids_list before the empty check, so update_metadata(None, ...) and update_documents(None, ...) raised TypeError where main returned an empty result, and disagreed with delete(None), which still did nothing. Skip the conversion for None in both. The None behaviour now matches LangChain's InMemoryVectorStore where it has the method — get_by_ids(None) raises TypeError, delete(None) does nothing, add_texts(ids=None) inserts — and, for update_* and upsert_documents which LangChain does not have, follows delete(None): nothing to update, or insert everything. A bare string is still refused. One test pins all six. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

minseokpark-CL force-pushed the fix/strict-item-ids branch from 88c3f06 to e50f535 Compare October 1, 2026 07:13

minseokpark-CL mentioned this pull request Oct 1, 2026

feat: add get_by_ids, reading documents back by item ID #21

Merged

Kim-YeongHyeon reviewed Oct 2, 2026

View reviewed changes

minseokpark-CL and others added 2 commits October 2, 2026 13:52

minseokpark-CL force-pushed the fix/strict-item-ids branch from e50f535 to ff04dc2 Compare October 2, 2026 04:54

minseokpark-CL merged commit 113f83b into feat/get-by-ids Oct 2, 2026

minseokpark-CL mentioned this pull request Oct 2, 2026

fix: refuse bool and float item IDs everywhere instead of coercing them #23

Merged

Navigation Menu

Repository navigation

fix: refuse bool and float item IDs everywhere instead of coercing them - #22

fix: refuse bool and float item IDs everywhere instead of coercing them#22
minseokpark-CL merged 2 commits into
feat/get-by-idsfrom
fix/strict-item-ids

minseokpark-CL commented Oct 1, 2026 •

edited

Loading

Uh oh!

Kim-YeongHyeon left a comment

Uh oh!

Kim-YeongHyeon Oct 2, 2026

Uh oh!

minseokpark-CL Oct 2, 2026

Uh oh!

Kim-YeongHyeon Oct 2, 2026

Uh oh!

minseokpark-CL Oct 2, 2026

Uh oh!

Kim-YeongHyeon Oct 2, 2026

Uh oh!

minseokpark-CL Oct 2, 2026

Uh oh!

minseokpark-CL commented Oct 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

minseokpark-CL commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

한국어

무엇이 문제였나

무엇이 달라지나

검증

English

What was wrong

What changes

Verification

Uh oh!

Kim-YeongHyeon left a comment

Choose a reason for hiding this comment

Uh oh!

Kim-YeongHyeon Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

minseokpark-CL Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

Kim-YeongHyeon Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

minseokpark-CL Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

Kim-YeongHyeon Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

minseokpark-CL Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

minseokpark-CL commented Oct 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

minseokpark-CL commented Oct 1, 2026 •

edited

Loading