Repository navigation
fix: refuse bool and float item IDs everywhere instead of coercing them - #23
Conversation
cokestrawberry
left a comment
There was a problem hiding this comment.
This review was generated by an AI agent under full delegation. It does not fulfill the mandatory secondary-owner review requirement. Merge judgment remains with a human reviewer.
| ``add_documents``, as ``str`` or ``int``. Any other value — ``bool`` and | ||
| ``float`` included — raises ``ValueError`` rather than being coerced onto | ||
| another item (``3.9`` would otherwise delete item 3). |
There was a problem hiding this comment.
🟡 ID 문자열 하나를 넘기면 글자마다 다른 item ID로 읽힙니다
ID 판별 논리는 맞아 보이나, ids에 list 대신 ID 문자열 하나를 넘기면(store.delete(doc.id)) 호출한 쪽이 지정하지 않은 행이 지워질 수 있습니다.
- 원인:
delete·get_by_ids·update_metadata·update_documents·upsert_documents·add_texts는ids가str인지 확인하지 않고 순회하므로, 문자열의 글자 하나하나가_readable_item_id를 통과하는 item ID가 됩니다. - 설명:
doc.id가"12"이면store.delete(doc.id)는 SDK에item_ids=[1, 2]를 넘겨 item 1과 2를 지우고,get_by_ids("12")도 item 1과 2를 돌려줍니다. b642543에도 있던 동작이지만 이 PR이 막으려는 결과와 같고, README.md:192의 예시 주석e.g. doc.id는doc.id를 그대로 넘기라는 뜻으로 읽힐 수 있습니다. - 수정: 위 메서드에서
ids를 순회하거나list(ids)로 바꾸기 전에str이나bytes이면TypeError를 내고, README 예시는store.delete([doc.id])로 적는 것을 제안합니다.
There was a problem hiding this comment.
맞습니다. 9fc0667 에서 ids 가 str / bytes / bytearray 면 일곱 호출 경로(delete, get_by_ids, update_metadata, update_documents 두 모드, upsert_documents, add_texts) 모두 TypeError 를 내고 SDK 를 부르지 않습니다. 메시지에 [doc.id] 를 적었고, README 의 delete 예시도 store.delete([doc.id]) 로 고쳤습니다. tuple·set·generator 는 그대로 받습니다. 테스트: test_ids_must_be_a_list_not_a_bare_string("13", "1", b"13", bytearray) 과 test_ids_accept_tuples_sets_and_generators.
cokestrawberry
left a comment
There was a problem hiding this comment.
#21을 검토하며 확인한 _readable_item_id의 문제가 이 PR의 head(ff04dc2)에도 그대로 있어 이 PR에 적습니다. 이 PR의 다른 변경은 이번 검토 범위에 넣지 않았습니다.
This review was generated by an AI agent under full delegation. It does not fulfill the mandatory secondary-owner review requirement. Merge judgment remains with a human reviewer.
| if isinstance(value, str): | ||
| text = value.strip() | ||
| if text.isascii() and text.isdigit(): | ||
| item_id = int(text) |
There was a problem hiding this comment.
🟡 4300자리를 넘는 숫자 문자열에서 int()가 먼저 예외를 냅니다
판별 규칙은 맞아 보이지만, 4300자리를 넘는 ASCII 숫자 문자열이 들어오면 메서드별 처리 규칙이 적용되기 전에 ValueError가 납니다.
- 원인:
_readable_item_id와_is_non_positive_integer는 자릿수를 확인하지 않고int(text)를 부르는데, CPython은 기본 설정에서 4300자리(sys.get_int_max_str_digits())를 넘는 문자열의 정수 변환을ValueError로 막습니다. - 설명: 그래서
"9" * 4301은 본문의 규칙과 달리get_by_ids에서 빠지지 않고 예외로 끝나며,add_texts의ids에서도 새 행과UserWarning대신 예외가 납니다.delete·update_*·upsert_documents는ValueError를 내기는 하지만, 메서드 이름이 붙은 메시지 대신Exceeds the limit (4300 digits) for integer string conversion메시지가 나갑니다. - 수정:
2**63 - 1이 19자리이므로, 두 함수에서int()를 부르기 전에 숫자 부분이 19자리 이하인지 확인하면 이런 값도 item ID가 아닌 값으로 분류되어 메서드별 규칙을 따릅니다.
There was a problem hiding this comment.
반영했습니다(9fc0667). _parse_integer 가 int() 전에 숫자 부분이 19자리 이하인지 보고, 넘으면 item ID 가 아닌 값으로 분류합니다. 그래서 "9"*4301 은 get_by_ids 에서 빠지고, delete 는 "expects integer item IDs" 메시지, add_texts 는 새 row + UserWarning 입니다. 테스트: test_digit_strings_longer_than_int64_are_not_ids(4301자리와 20자리).
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>
ff04dc2 to
03c4e6f
Compare
…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>
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>
한국어
무엇이 문제였나
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 다.무엇이 달라지나
_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 에는 Pythonint로 넘긴다.delete/update_metadata/update_documents/upsert_documents는ValueError를 내고 SDK 를 부르지 않는다.get_by_ids는 결과에서 뺀다.add_texts/add_documents의ids는 둘로 나뉜다: item ID 가 아닌 문자열(UUID,"doc-7", 0 부터 시작하는 chunk 번호"0","+3", 비ASCII 숫자)은 지금처럼 새 row 로 넣고UserWarning을 낸다 — LangChain 의 ID 는 문자열이라 외부 ID 일 수 있다. 반면 어떤 ID 도 될 수 없는 타입(float와 그 밖의 비정수 수,bool, bytes,np.array(3)같은__index__객체)은ValueError를 낸다 — 잘못 캐스팅된 item ID 일 가능성이 높고(pandas 정수 열은 NaN 하나로 float64 가 된다), 조용히 새 row 를 넣으면 재적재마다 문서가 중복되기 때문이다(리뷰 반영).ids자체가str/bytes/bytearray면 모든 메서드가TypeError를 낸다(리뷰 반영).str은 글자 하나하나가Sequence[str]의 원소라delete("13")이 item 1 과 3 을,delete(b"13")이 item 49 와 51 을 지웠다. main 과 LangChain 의InMemoryVectorStore에도 같은 동작이 있다. tuple·set·generator 는 그대로 받는다. README 의delete예시를store.delete([doc.id])로 고쳤다.ids=None은 LangChain 의InMemoryVectorStore와 같다:get_by_ids(None)은TypeError,delete(None)은 아무 일 없음,add_texts(ids=None)은 새로 추가. LangChain 에 없는update_metadata/update_documents는delete(None)처럼 빈 결과,upsert_documents(ids=None)은 전부 추가(main 과 같음)._parse_integer하나로 한다(리뷰 반영):bool이 아닌numbers.Integral, 또는 앞에-를 허용한 19자리 이하 ASCII 숫자 문자열. 0 과 음수는 "item IDs are positive integers up to 2**63-1" 메시지, 그 밖("+3","3_000", 4301자리 숫자 문자열 — CPython 은 4300자리 넘는int()를 자체 오류로 막는다)은 "expects integer item IDs" 메시지다._is_non_positive_integer는 없앴다."٣"같은 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 은 그 판별을 모든 메서드에 적용한다.int(...)로 통과하던 값이 더는 item ID 가 아니다:"+3","3_000", 전각"3",b"3",Decimal(3),Fraction(3),np.array(3),3.0.delete/update_*/upsert_documents에 넘기면ValueError,get_by_ids는 빼며,add_texts는 문자열이면 새 row + 경고, 그 밖의 타입이면ValueError다. 받는 형식은 "add_texts가 돌려준 문자열, 또는 Python·NumPy 정수" 로 README Limitations 에 적었다.delete와add_texts의 docstring 에 받는 형식과 위 규칙을 적었다.동작 비교:
idsidsTrue,3.9,3.0,np.float64(3.0)ValueErrorValueErrorb"3",Decimal(3),Fraction(3),np.array(3)ValueErrorValueError"٣","3","+3","3_000"ValueErrorUserWarning"0",0,"-3",2**63,"9"*4301ValueError(4301자리는 CPython 메시지)ValueError(우리 메시지)UserWarning(4301자리는 CPython 오류)UserWarning3,"3"," 3 ",np.int64(3),2**63-1ids="13"(리스트 아님)TypeErrorTypeErrorids=b"13"TypeErrorTypeError검증
python -m pytest tests -m "not integration"—d5e4514에서 168 passed. 새 guard 13개를 하나씩 약하게 바꿔 각각 테스트가 실패하는 것을 확인했다. 새 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를 내는 것,ids="13"·b"13"이 일곱 호출 경로 모두에서 SDK 호출 없이TypeError인 것, tuple·set·generator 는 받는 것, 4301자리·20자리 숫자 문자열이 ID 가 아닌 것,add_texts가True·3.9·3.0·Decimal·Fraction·np.array(3)·np.float64에ValueError를 내고 item 3 을 건드리지 않는 것,"0"·"-3"·"+3"·"3.0"·"3_000"·"٣"·UUID 문자열은 새 row + 경고인 것,"-0"은 positive-integers 메시지이고"+0"·"+3"은 expects-integer 메시지인 것,ids=None의 여섯 메서드 동작.English
What was wrong
delete,update_metadata,update_documents,upsert_documentsand theidsofadd_texts/add_documentsturned each ID intoint(...)directly.int(True)is 1;int(3.9)andint(3.0)are 3.delete(ids=[3.9])deleted item 3, andupdate_documents([3.9], ...)andadd_texts(..., ids=[3.9])overwrote item 3 — rows the caller never named.What changes
_readable_item_id(added in feat: add get_by_ids, reading documents back by item ID #21 forget_by_ids). An item ID is a positive integer (anynumbers.Integral, so NumPy integers such asnp.int64count) or its ASCII decimal string only;bool/np.bool_andfloat/np.float64are not item IDs. The SDK receives a plain Pythonint.delete/update_metadata/update_documents/upsert_documentsraiseValueErrorand do not call the SDK.get_by_idsleaves it out. Theidsofadd_texts/add_documentssplit in two: a string that is not an item ID (a UUID,"doc-7", a 0-based chunk number"0","+3", non-ASCII digits) still inserts a new row with aUserWarning— LangChain IDs are strings, so it may be a foreign ID — while a value of a type that can be no ID at all (floatand other non-integral numbers,bool, bytes,__index__objects such asnp.array(3)) raisesValueError, because it is almost always a wrongly-typed item ID (a pandas integer column turns float64 on its first NaN) and a silent new row would duplicate the document on every re-load (from review).TypeErrorwhenidsitself is astr/bytes/bytearray(from review). Astris aSequence[str]of its characters, sodelete("13")deleted items 1 and 3 anddelete(b"13")items 49 and 51 — on main, and in LangChain's ownInMemoryVectorStore. Tuples, sets and generators are still accepted. The READMEdeleteexample now passes[doc.id].ids=Nonematches LangChain'sInMemoryVectorStore:get_by_ids(None)raisesTypeError,delete(None)does nothing,add_texts(ids=None)inserts. Forupdate_metadata/update_documents, which LangChain does not have,Nonereturns an empty result likedelete(None), andupsert_documents(ids=None)inserts everything (as on main)._parse_integer, does the check (from review): a non-boolnumbers.Integral, or an ASCII digit string of at most 19 digits with an optional leading-. Zero and negatives get the "item IDs are positive integers up to 2**63-1" message; everything else ("+3","3_000", a 4301-digit string — CPython refusesint()above 4300 digits with its own error) gets "expects integer item IDs"._is_non_positive_integeris gone."٣"and values of2**63or more are not item IDs either. The same hole in feat: add get_by_ids, reading documents back by item ID #21'sget_by_idswas 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.int(...)accepted on main are no longer item IDs:"+3","3_000", fullwidth"3",b"3",Decimal(3),Fraction(3),np.array(3),3.0.delete/update_*/upsert_documentsraiseValueError,get_by_idsleaves them out, andadd_textsinserts a new row with a warning for a string or raises for any other type. The accepted forms — the stringsadd_textsreturned, or Python / NumPy integers — are stated in README Limitations.deleteandadd_textsdocstrings now state the accepted forms and the rules above.Behaviour, before and after:
idsidsTrue,3.9,3.0,np.float64(3.0)ValueErrorValueErrorb"3",Decimal(3),Fraction(3),np.array(3)ValueErrorValueError"٣","3","+3","3_000"ValueErrorUserWarning"0",0,"-3",2**63,"9"*4301ValueError(CPython's message for 4301 digits)ValueError(ours)UserWarning(CPython error for 4301 digits)UserWarning3,"3"," 3 ",np.int64(3),2**63-1ids="13"(not a list)TypeErrorTypeErrorids=b"13"TypeErrorTypeErrorVerification
python -m pytest tests -m "not integration"— 168 passed ond5e4514. Each of the 13 new guards was weakened one at a time and a test failed every time. New unit tests:deleteraisesValueErrorforTrue,3.9,3.0and"٣"without calling the SDK delete;update_metadata,update_documentsandupsert_documentsraiseValueErrorfor the same values without calling the SDK;add_textsinserts those as new rows and leaves item 3 untouched; ints and decimal strings still work; zero and negatives keep their message;get_by_idsleaves out"٣"; NumPy integers (int64,int32,uint64) are read as item IDs whilenp.bool_andnp.float64are left out or raiseValueError;ids="13"andb"13"raiseTypeErroron all seven call paths without an SDK call; tuples, sets and generators are accepted; 4301- and 20-digit strings are not IDs;add_textsraises forTrue,3.9,3.0,Decimal,Fraction,np.array(3)andnp.float64and leaves item 3 alone;"0","-3","+3","3.0","3_000","٣"and a UUID string insert a new row with a warning;"-0"gets the positive-integers message while"+0"and"+3"get expects-integer;ids=Noneacross the six methods.🤖 Generated with Claude Code